align snapshot_download to respect hfh latest version

[sayakpaul]

What does this PR do?

Fixes #14117

@BenjaminBossan does this work for you?

[BenjaminBossan]

The PEFT tests succeed on this branch, thanks a lot.

(reminder to myself: testing with pytest "tests/test_stablediffusion.py::TestStableDiffusionModel::test_disable_adapter" "tests/test_tuners_utils.py::TestPeftCustomKwargs::test_maybe_include_all_linear_layers_diffusion" -v)

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

[BenjaminBossan]

I can't comment on the logic of the fix, but it resolves the PEFT errors. Thanks.

[BenjaminBossan]

Changes seem to be unrelated.

[BenjaminBossan]

A question: Is there going to be a patch release with this fix? If not, I need to prepare a fix on PEFT to exempt the failing tests until Diffusers 0.40 is released.

[sayakpaul]

It's a breaking change from hfh and not diffusers. So, maybe you will need to wait till the next release.

[Wauplin]

This PR feels correct 👍 (sorry for the delay reviewing it)

Though for a broader context I still do not understand why the allow_pattern/ignore_pattern arguments are different depending on the Hub availability

It's a breaking change from hfh and not diffusers. So, maybe you will need to wait till the next release.

It's not considered as a breaking change on hfh-side so it won't be fixed in next releases no

[Wauplin]

hacky but "works" though you don't benefit from the new "raise if incomplete snapshot"-feature i.e. if a previous download failed mid-way it won't be caught here, meaning a separate error will be raised downstream (likely when trying to load the model) - and the base exception will have been lost in the process

[Wauplin]

(sorry just wanted to "comment" instead of "approve" -not well versed with diffusers internals- but the summary is the same^^)

[BenjaminBossan]

It's a breaking change from hfh and not diffusers. So, maybe you will need to wait till the next release.

It's not considered as a breaking change on hfh-side so it won't be fixed in next releases no

I took this to mean that it will have to wait until the next Diffusers (minor) release, which will contain this fix, as there won't be a Diffusers patch release for this.

[sayakpaul]

It's not considered as a breaking change on hfh-side so it won't be fixed in next releases no

Next release of Diffusers. I am not sure why it's not considered a breaking change on HFH side because it clearly broke things in Diffusers as we can see. From what I understand, the behaviour of the underlying HFH-side functions also changed. Maybe I am missing something.

[Wauplin]

. I am not sure why it's not considered a breaking change on HFH side because it clearly broke things in Diffusers as we can see.

Mostly because it's considered as bug fix (I know it's mostly semantic^^)

because it clearly broke things in Diffusers as we can see

did it broke things only in the tests or in real life? I feel like if we are now raising an error for an incomplete download it's better than having diffusers raise because a file is not found in a local snapshot that was supposed to contain it

[Wauplin]

requesting to download a single file to then use Path(...).parent is IMO a hacky way of getting the snapshot_folder. (hence why it needs 7 lines of explanation in comments)

to avoid that I've opened huggingface/huggingface_hub#4500 to expose the snapshot_folder in the error in case of IncompleteSnapshotError. I think this is the most explicit way to do things. In diffusers the code will look like this:

try:
    snapshot_path = snapshot_download(...)
except IncompleteSnapshotError as error:
    incomplete_snapshot_path = error.snapshot_path

making it explicit for anyone reading the code that the folder is incomplete and that we knowingly ignored it

(even though, once again, I do think the best way to tackle this is to provide the correct allow_pattern/ignore_pattern filters in the first place. I still don't understand why it's not the solution we are trying to have in this PR. If there is a rational behind not trying to fix the root cause I genuinely believe it should at the very least be mentioned/documented somewhere)

[sayakpaul]

Thanks for offering further thoughts. Post-mortem:

The reason we can't write the patterns down without any file listing: they're data-dependent. allow_patterns are largely concrete filenames chosen by variant_compatible_siblings (variant / sharded / safetensors-vs-bin resolution), and _get_ignore_patterns picks formats based on what actually exists in the repo.

For the online path, that listing comes from model_info().siblings. For the offline path, the only faithful source is the cached snapshot itself, which holds exactly what the same computation downloaded when online — so both modes compute the same patterns. We also test it here:

diffusers/tests/pipelines/test_pipelines.py

Line 508 in e92e259

def test_local_files_only_uses_same_snapshot_download_patterns(self):

More precisely, the offline path falls through into the same pattern-computation code as online, fed with a filenames' set that came from the cache instead of model_info().siblings. Before this PR, the offline path skipped all of this (patterns stayed None).

Regarding hf_hub_download(config_file), the online branch makes the identical call because model_index.json's content dictates the computation (component folders, _ignore_files, custom pipeline):

diffusers/src/diffusers/pipelines/pipeline_utils.py

Lines 1655 to 1664 in e92e259

	if not local_files_only:
	config_file = hf_hub_download(
	pretrained_model_name,
	cls.config_name,
	cache_dir=cache_dir,
	revision=revision,
	proxies=proxies,
	force_download=force_download,
	token=token,
	)

(All of that is used for computing the patterns)

For the offline path, we are also reusing its parent as the snapshot dir, matching the pre-existing snapshot_folder = Path(config_file).parent on main.

huggingface/huggingface_hub#4500 is cool, thanks! snapshot_path on the error does locate the folder correctly. But the error only fires from snapshot_download.We need the folder before calling snapshot_download (its listing is the input to the pattern computation). Using it would mean a deliberate no-patterns call that fails on every offline diffusers load just to catch the error.

Maybe there's a way to refactor this but that's for another PR.

[Wauplin]

Thanks for the detailed response! That makes sense then. So if I understand correctly what's missing before-hand is the list of files in the repo when in offline mode, right? Luckily we now have this information in cache -hence why we are able to tell if a folder is incomplete-. We could definitely add a method to retrieve that cached list to that you can:

1. make a repo info call to get repo.siblings
2. if that fails, get repo.siblings from the cache (requires a method from huggingface_hub)
3. and then compute the allow/ignore patterns from the list of files, no matter if you are in online or offline mode

Would that be interesting for you?

Using it would mean a deliberate no-patterns call that fails on every offline diffusers load just to catch the error.

Yes that's what I had in mind. I feel that it's less hacky than a deliberate hf_hub_download call on the index file, just to do a Path(index file).parent afterwards.

[Wauplin]

opened huggingface/huggingface_hub#4513 to add a get_cached_repo_tree method useful when repo.siblings or list_repo_tree fails (in offline mode). A priori, this is the cleanest way to determine the patterns down the line (with variants, etc.) without raising an error and without skipping the "incomplete folder check". Should be shipped today or tomorrow with the rest.