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

Fix for fetching variants only #10646

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Fix for fetching variants only #10646

wants to merge 7 commits into from

Conversation

DN6
Copy link
Collaborator

@DN6 DN6 commented Jan 24, 2025

What does this PR do?

With PR: #9869 we fixed downloading sharded variants only and mixed variants, however we missed the case where a component might have sharded non-variant files and non-sharded variant file, which is the case with issue: #10634

This PR:

  1. Adds a simpler has_variant check and removes the previous checks. This check first checks to see if we are in a component folder and then checks if any variants exist within the folder. If no folder exists then skip trying to add the additional non variant file.
  2. Adds additional tests to check for the condition mentioned in issue The huggingface repo need to be fixed for Sana 2K and 4K models #10634.

Since usable_filenames is always populated with variants, we capture the necessary variant files and what we're trying to avoid is extra file downloads.

The only edge case I can think of here where this would fail (which passes with the current implementation) is if the filenames are the following:

Non-Variant in the main dir and a variant in a subfolder. Although I think this an edge case that we probably can't load anyway? I can't think of any pipelines that would have this configuration.

filenames = ["diffusion_pytorch_model.safetensors", f"unet/diffusion_pytorch_model.{variant}.safetensors"]

Fixes # (issue)
#10634

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.

@DN6 DN6 requested a review from yiyixuxu January 24, 2025 19:38
@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.

@lawrence-cj
Copy link
Contributor

Thanks @DN6 for supporting!

for filename in non_variant_filenames:
if convert_to_variant(filename) in variant_filenames:
continue
return any(f.startswith(component) for f in variant_filenames)
Copy link
Collaborator

@yiyixuxu yiyixuxu Jan 28, 2025

Choose a reason for hiding this comment

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

what happends if like we only have a bf16.bin and this is a non-variant safetensors?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As in we are trying to fetch something like this?

        variant = "fp16"
        filenames = [
                f"vae/diffusion_pytorch_model.{variant}.bin",
                f"text_encoder/model.{variant}.bin",
                f"unet/diffusion_pytorch_model.{variant}.bin",
        ]
        model_filenames, variant_filenames = variant_compatible_siblings(filenames, variant=None)

Copy link
Collaborator

Choose a reason for hiding this comment

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

like this, I think we should fetch the non-variant safetensors in this case, no?

    variant = "fp16"
    filenames = [
            f"vae/diffusion_pytorch_model.{variant}.bin",
            f"text_encoder/model.{variant}.bin",
            f"unet/diffusion_pytorch_model.{variant}.bin",
            f"vae/diffusion_pytorch_model.safetensors",
            f"text_encoder/model.safetensors",
            f"unet/diffusion_pytorch_model.safetensors",
    ]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm currently the behaviour on main is to return all the files in that list (both bin and safetensors) as usable_filenames and I think the ignore patterns would remove the bin files, resulting in just the safetensors being downloaded.

With this change only the fp16.bin files would be downloaded. Which feels technically "correct" to me since they are the "variant" files of each component. IMO non-variants should only be downloaded if no variant exists (regardless of format)

But this case implies that the proposal here is a breaking change, so I'll update to account for it.

@@ -104,7 +104,7 @@ def is_safetensors_compatible(filenames, passed_components=None, folder_names=No
extension is replaced with ".safetensors"
"""
passed_components = passed_components or []
if folder_names is not None:
if folder_names:
Copy link
Collaborator Author

@DN6 DN6 Jan 29, 2025

Choose a reason for hiding this comment

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

This change is needed to correct a weird bug that was caught with this test
tests/pipelines/test_pipelines.py::CustomPipelineTests::test_load_custom_github

What was basically happening was that variant_compatible_siblings was incorrectly returning both the bin and safetensors version of the checkpoint in this repo
https://huggingface.co/google/ddpm-cifar10-32/tree/main

In the DiffusionPipeline download method, if model_folder_names ends up being an empty set

model_folder_names = {os.path.split(f)[0] for f in model_filenames if os.path.split(f)[0] in folder_names}
and is passed to is_safetensors_compatible the function will return False because filenames will end up being an empty set. But since variant_compatible_siblings was asking to fetch both bin and safetensors versions of the checkpoint, the test passed because the bin version was downloaded.

@DN6
Copy link
Collaborator Author

DN6 commented Jan 29, 2025

@yiyixuxu Had to do a bit more of a refactor to account for safetensors prioritization. But it should be much more robust to handle any number of repo file combinations. I've added a test for your case and a few others as well. I think they should cover all likely repo file layout scenarios, but if there are others I may have missed lmk.

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