-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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
🧼 remove v4.44 deprecations #34245
base: main
Are you sure you want to change the base?
🧼 remove v4.44 deprecations #34245
Conversation
logger.warning( | ||
"Note that `shard_checkpoint` is deprecated and will be removed in v4.44. We recommend you using " | ||
"split_torch_state_dict_into_shards from huggingface_hub library" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(followed this instruction whenever shard_checkpoint
was used)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this is not the exact function. We can't just replace it directly. You need to build the shard and the index again using the output of this function. Check the example here: https://huggingface.co/docs/huggingface_hub/main/en/package_reference/serialization#huggingface_hub.split_torch_state_dict_into_shards.example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing but the processing logic refactor wen longer than I expected. We are still doing the last bits in #33424 and the checkpoints on the hub are not updated yet (tracker #33374). So we can't be raising ValueError yet
Maybe we can change the deprecation version to be after a few more minor releases. We have also same warnings in all llava and blip models afair. Also now I think we can even do more than just a few releases, since the changes will invalidate many models on the hub that are not owned by us. WDYT, which version we can put in the message?
We should then support it for as long as possible :) If it is critical to any popular model, I would not even set a deprecation version target at all. Leave the exception without target, and set a target when it blocks you from doing something OR if it is causing many issues. An example of this is supporting If it is only used in some niche models, and applying the deprecation (raising exception) is a significant win for transformers, then I would set at least 6 minor versions. LMK which one is it, and I will update the warning. |
I hope you meant I would say this particular one is a niche model, given download stats from the hub. And most older llava and BLIP models may be outdated fast as we get newer and better releases each month. But I think we still can't enforce users to do something by elevating this to an Exception. My preference is to keep it for 6 minor versions as a warning, and try to update official models on the hub until that. We have write access to all of them afaik except for Video-LLaVA so updating will be straightforward |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update ! I left a comment. Also, I check that we need to enforce huggingface-hub to 0.24.0 because of this commit. we accept max_shard_size
with string format only from this version.
logger.warning( | ||
"Note that `shard_checkpoint` is deprecated and will be removed in v4.44. We recommend you using " | ||
"split_torch_state_dict_into_shards from huggingface_hub library" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this is not the exact function. We can't just replace it directly. You need to build the shard and the index again using the output of this function. Check the example here: https://huggingface.co/docs/huggingface_hub/main/en/package_reference/serialization#huggingface_hub.split_torch_state_dict_into_shards.example
@zucchini-nlp haha yes, |
dcd2588
to
48f5982
Compare
@zucchini-nlp @SunMarc PR comments addressed, LMK if they look good to you :) |
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. |
What does this PR do?
See title :)
@zucchini-nlp -> video llava changes
@SunMarc -> all other changes