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.

Sign up for GitHub

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

Jump to bottom

Respect resume_download deprecation #30620

Merged
merged 3 commits into from
May 6, 2024
Merged

Respect resume_download deprecation #30620

merged 3 commits into from
May 6, 2024

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented May 2, 2024

Fix #30618 (cc @albertvillanova).

resume_download has been deprecated in huggingface_hub==0.23.0 release (see huggingface/huggingface_hub#2223). All downloads are now resumed by default. This PR switches all default values to None instead of False which will prevent from having a warning message. If a user manually sets resume_download to True/False, then they'll still get the FutureWarning from huggingface_hub which is the expected behavior.

We will need to make sure to completely remove resume_download from transformers before dropping it in huggingface_hub. However, this is not expected mid-term (at least not before hfh==1.0)

Technically this PR would be compatible with previous versions of huggingface_hub since False and None were processed the same way but I preferred to update requirements to make it compliant with type annotations.

@Wauplin Wauplin requested a review from a team May 2, 2024 14:53
@Wauplin Wauplin mentioned this pull request May 2, 2024
Closed
albertvillanova
albertvillanova reviewed May 2, 2024
View reviewed changes
Copy link
Member

@albertvillanova albertvillanova 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 the fix of the warning, which makes datasets CI fail.

@ydshieh
Copy link
Collaborator

ydshieh commented May 2, 2024

Thank you @Wauplin !

So the goals of this PR are:

  • avoid warning (by set the default to None)
  • update the docstring to show it is deprecated

Right?

@HuggingFaceDocBuilderDev
Copy link

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.

@Wauplin
Copy link
Contributor Author

Wauplin commented May 2, 2024

So the goals of this PR are:

  • avoid warning (by set the default to None)
  • update the docstring to show it is deprecated

Right?

Correct!

ydshieh
ydshieh approved these changes May 2, 2024
View reviewed changes
Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

Sound good to me!

@ydshieh
Copy link
Collaborator

ydshieh commented May 2, 2024

well as long as CI pass

@Wauplin
Copy link
Contributor Author

Wauplin commented May 2, 2024

well as long as CI pass

Is it related to my PR? 🙈 😭

@ydshieh
Copy link
Collaborator

ydshieh commented May 2, 2024

my ultimate solution for this is to re-run until is ✅ - just triggered

This was referenced May 2, 2024
Merged
Merged
@ydshieh
Copy link
Collaborator

ydshieh commented May 2, 2024

All good!

@Wauplin
Copy link
Contributor Author

Wauplin commented May 2, 2024

Thanks @ydshieh! All good to merge it then? :)

amyeroberts
amyeroberts reviewed May 2, 2024
View reviewed changes
Copy link
Collaborator

@amyeroberts amyeroberts 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 updating everything!

I just put in suggestions to make the docstrings in the "transformers format", specifically we don't say anything about the default if it's None

src/transformers/dynamic_module_utils.py Outdated Show resolved Hide resolved
src/transformers/dynamic_module_utils.py Outdated Show resolved Hide resolved
src/transformers/feature_extraction_utils.py Outdated Show resolved Hide resolved
src/transformers/generation/configuration_utils.py Outdated Show resolved Hide resolved
src/transformers/image_processing_utils.py Outdated Show resolved Hide resolved
src/transformers/models/wav2vec2/modeling_wav2vec2.py Outdated Show resolved Hide resolved
src/transformers/tokenization_utils_base.py Outdated Show resolved Hide resolved
src/transformers/utils/hub.py Outdated Show resolved Hide resolved
src/transformers/utils/peft_utils.py Outdated Show resolved Hide resolved
src/transformers/utils/hub.py Outdated Show resolved Hide resolved
@Wauplin
Copy link
Contributor Author

Wauplin commented May 3, 2024

@amyeroberts I fixed the docstrings as discussed in 2eba9af :)

@Wauplin Wauplin requested a review from amyeroberts May 3, 2024 06:15
@Wauplin
Copy link
Contributor Author

Wauplin commented May 3, 2024

CI is green! @amyeroberts I'll let you merge it if changes are fine for you :)

ArthurZucker
ArthurZucker reviewed May 6, 2024
View reviewed changes
Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Amy is out, can you rebase so that I can merge? 🤗

LysandreJik
LysandreJik approved these changes May 6, 2024
View reviewed changes
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.

Thanks!

@LysandreJik LysandreJik merged commit 835de4c into main May 6, 2024
20 of 23 checks passed
@LysandreJik LysandreJik deleted the deprecate-resume-download branch May 6, 2024 16:01
@szwang-seedtech szwang-seedtech mentioned this pull request May 9, 2024
Closed
zucchini-nlp pushed a commit to zucchini-nlp/transformers that referenced this pull request May 10, 2024
@Wauplin @LysandreJik @zucchini-nlp
Respect resume_download deprecation (huggingface#30620)
0321d12 
* Deprecate resume_download

* remove default resume_download value

---------

Co-authored-by: Lysandre Debut <[email protected]>
@aymenkrifa aymenkrifa mentioned this pull request May 13, 2024
Closed
itazap pushed a commit that referenced this pull request May 14, 2024
@Wauplin @LysandreJik
Respect resume_download deprecation (#30620)
d6709d8 
* Deprecate resume_download

* remove default resume_download value

---------

Co-authored-by: Lysandre Debut <[email protected]>
@tomaarsen tomaarsen mentioned this pull request May 22, 2024
Open
@SuperJessBerlin
Copy link

Hi, i got the problem that i can't get huggingface to download models or something. Yesterday it totally worked fine. Today it tells me Future Warning: resume_download is deprecated and will be removed in version 1.0.0.

Can you help me?

@amyeroberts
Copy link
Collaborator

Hi @SuperJessBerlin,

There's two possible things here. With regards to the message, yes, this argument has been deprecated, all downloads resume by default. You can pass the argument in, but it won't have any effect.

With regards to downloading models, when you say "i can't get huggingface to download models or something", have you observed that models aren't downloading at all?

@SuperJessBerlin
Copy link

i tried this morning and it didn't do to any model. I then installed it all new and it will not get this one model not to work.
Error

@amyeroberts
Copy link
Collaborator

@SuperJessBerlin The warning is there to inform you that the argument resume_download is deprecated i.e. it shouldn't be set to True/False as it will have no effect. Seeing this warning doesn't mean downloads aren't happening however. I can see from the screen shot, this is being used through the stable diffusion webui, which should still work.

@SuperJessBerlin
Copy link

i get to the web ui.....but it loads unlimited and never would let me create something.

@SuperJessBerlin
Copy link

Error

and it would load for unlimited time

@SuperJessBerlin
Copy link

oh and it has a new overlay. Don't know that yet.

@amyeroberts
Copy link
Collaborator

@SuperJessBerlin Just from a screenshot alone, I wouldn't be able to tell you what is (or is not) happening. I suspect it might just be taking a while to generate an image. Regardless, this is unrelated to this PR. I'd suggest opening an issue on the stable diffusion web UI repo

@albertvillanova albertvillanova mentioned this pull request May 24, 2024
Merged
unscrew added a commit to unscrew/transformers that referenced this pull request Jun 9, 2024
@unscrew
Apply the change deprecating 'resume_download' param to unchanged lin…
d8f28a7 
…es to be consistent across the source code

- Apply the change made in PR huggingface#30620
@unscrew unscrew mentioned this pull request Jun 9, 2024
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ArthurZucker ArthurZucker ArthurZucker left review comments

@albertvillanova albertvillanova albertvillanova left review comments

@LysandreJik LysandreJik LysandreJik approved these changes

@ydshieh ydshieh ydshieh approved these changes

@amyeroberts amyeroberts Awaiting requested review from amyeroberts

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

FutureWarning about resume_download is raised after huggingface-hub 0.23.0 release
8 participants
@Wauplin @ydshieh @HuggingFaceDocBuilderDev @SuperJessBerlin @amyeroberts @albertvillanova @LysandreJik @ArthurZucker