Skip to content

Improve downloads of sharded variants #9869

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

Merged
merged 6 commits into from
Nov 8, 2024
Merged

Improve downloads of sharded variants #9869

merged 6 commits into from
Nov 8, 2024

Conversation

DN6
Copy link
Collaborator

@DN6 DN6 commented Nov 5, 2024

What does this PR do?

Our current check for variants in variant_compatible_sibling downloads extra non variant files. See: #9253 (comment)

This is because we check if a non-variant filename has an equivalent variant filename. e.g. transformers/model-0003-0005.safetensors has transformers/model-fp16-0003-0005.safetensors in its variants. If it doesn't, we add the file to the list of allowed patterns to download.

This type of check doesn't work for sharded checkpoints since variants of smaller dtypes such as fp16 will always have a fewer number of shards. Therefore the non-variant shard file have no equivalent variant shard file.

This PR:
Changes the check to see if a variant exists for a pipeline component (regardless of whether it's sharded or not). If a variant exists for the component, we don't attempt to add any more files related to that component.

Fixes # (issue)

Before submitting

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@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.

@DN6
Copy link
Collaborator Author

DN6 commented Nov 5, 2024

Oh we should also account for model repos without component folders. Updating.

Comment on lines 203 to 206
if not len(filename.split("/")) == 2:
continue
component, component_filename = filename.split("/")
components_with_variant.add(component)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if not len(filename.split("/")) == 2:
continue
component, component_filename = filename.split("/")
components_with_variant.add(component)
components_with_variant.add(filename.split("/")[0])

I think we should match the logic to find "component" between variant_filenames and non_variant_filenames

Copy link
Collaborator

Choose a reason for hiding this comment

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

ohh I realize you actually just updated this to account for files that are not put inside any subfolders
maybe make this a in-line function find_component or something and do the same the for f in non_variant_filenames too

for f in non_variant_filenames:
variant_filename = convert_to_variant(f)
if variant_filename not in usable_filenames:
component = f.split("/")[0]
Copy link
Member

Choose a reason for hiding this comment

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

Will this work in case we're loading from a non-subfolder location?

@DN6
Copy link
Collaborator Author

DN6 commented Nov 6, 2024

@yiyixuxu Made it a bit more robust by using the existing regexes and sharded variant index files.

Now we

  1. First check if a variant version of a non-variant file exists in variant files names. If this is the case skip adding the file
  2. If a variant version of a non-variant file does not exist. First check if its in a component folder and check if a sharded variant index file exists in that folder. If it isn't a component folder check the main dir. If a sharded variant index file is present then skip
  3. Only add a file if none of the above is True.

Copy link
Collaborator

@yiyixuxu yiyixuxu left a comment

Choose a reason for hiding this comment

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

awesome!! thank you

@sayakpaul
Copy link
Member

@DN6 I think it'd be nice to also add a test for this improvement. Great work!

@DN6 DN6 merged commit 1b39254 into main Nov 8, 2024
18 checks passed
sayakpaul added a commit that referenced this pull request Dec 23, 2024
* update

* update

* update

* update

---------

Co-authored-by: Sayak Paul <spsayakpaul@gmail.com>
@DN6 DN6 mentioned this pull request Jan 24, 2025
6 tasks
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.

4 participants