Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optionally enable export if not exported model provided #722

Closed
wants to merge 4 commits into from

Conversation

eaidova
Copy link
Collaborator

@eaidova eaidova commented May 21, 2024

What does this PR do?

Fixes # (issue)

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@eaidova eaidova force-pushed the ea/optional_export_flag branch from 4e1b57d to a7cfa5d Compare May 23, 2024 15:27
@eaidova eaidova force-pushed the ea/optional_export_flag branch from 82ba6cd to ff1a2e0 Compare May 23, 2024 16:37
@eaidova eaidova marked this pull request as ready for review May 23, 2024 16:44
@ilya-lavrenov
Copy link

@echarlaix @AlexKoff88 @helena-intel could you please review and merge before next optimum release?

trust_remote_code=trust_remote_code,
)

if export is None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better way to introduce a separate variable and do the same like:

need_export = export or cls._check_export_status(model_id, revision, subfolder, cache_dir, local_files_only or HF_HUB_OFFLINE)

@AlexKoff88
Copy link
Collaborator

I don't have strong objections but I wonder if there is a more elegant way to do the same without introducing so much code that mostly duplicated Transformers logic. @echarlaix should know it better.

Also a test is required.

@echarlaix
Copy link
Collaborator

echarlaix commented Jun 3, 2024

I don't have strong objections but I wonder if there is a more elegant way to do the same without introducing so much code that mostly duplicated Transformers logic. @echarlaix should know it better.

Also a test is required.

Yes I think we can simplify this by using find_files_matching_pattern like :

pattern = r"(.*)?openvino(.*)?\_model.xml"
ov_files = find_files_matching_pattern(
    model_name_or_path,
    pattern,
    subfolder=subfolder,
    use_auth_token=token,
    revision=revision,
)

export = len(ov_files) == 0

also used in #740 for the openvino pipelines

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants