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

Improve snapshot_download offline mode #1913

Merged
merged 6 commits into from
Dec 19, 2023

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Dec 15, 2023

Follow-up after #1899 and huggingface/diffusers#6168 (comment).

At the moment if snapshot_download fails because of a connection error, because repo not found, because Hub is down, etc., we do not try to locate the cached snapshot directory. This is not consistent with 1. what happens in hf_hub_download and 2. what happens in snapshot_download itself if local_files_only/HF_HUB_OFFLINE is set. This PR fixes this by adapting the existing workflow of hf_hub_download to snapshot_download. It should help diffusers users that have an unstable connection to load their weights when already cached.

Note: it should help but once this is shipped we would still need to adapt diffusers implementation as well. At the moment diffusers make at least a model_info to check file names (if I remember correctly). This call would have to handle the OfflineModeIsEnabled error as well.

Copy link
Member

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

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

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Looks awesome! Just left a comment regarding an error I don't understand completely.

LGTM

src/huggingface_hub/_snapshot_download.py Show resolved Hide resolved
src/huggingface_hub/_snapshot_download.py Outdated Show resolved Hide resolved
@Wauplin
Copy link
Contributor Author

Wauplin commented Dec 19, 2023

Thanks for the thorough review! I've updated the error message that was wrong. I'll wait for the CI to complete and merge it to include it in next release :)

@Wauplin
Copy link
Contributor Author

Wauplin commented Dec 19, 2023

Failing test is unrelated. Let's get it merged!

@Wauplin Wauplin merged commit 1ab9308 into main Dec 19, 2023
15 of 16 checks passed
@Wauplin Wauplin deleted the improve-snapshot-download-offline-mode branch December 19, 2023 09:06
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