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

🧼 remove v4.44 deprecations #34245

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

Conversation

gante
Copy link
Member

@gante gante commented Oct 18, 2024

What does this PR do?

See title :)

@zucchini-nlp -> video llava changes
@SunMarc -> all other changes

Comment on lines -400 to -402
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"
Copy link
Member Author

@gante gante Oct 18, 2024

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)

Copy link
Member

@SunMarc SunMarc Oct 18, 2024

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

Copy link
Member

@zucchini-nlp zucchini-nlp 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 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?

@gante
Copy link
Member Author

gante commented Oct 18, 2024

since the changes will invalidate many models on the hub that are not owned by us.

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 model.config modifications to parameterize generate -- it is super inconvenient for maintenance, but many examples/models rely on it.

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.

@zucchini-nlp
Copy link
Member

I hope you meant warning, not exception. I don't think we should be raising exception unless at least the official checkpoint is updated accordingly.

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

Copy link
Member

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

Comment on lines -400 to -402
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"
Copy link
Member

@SunMarc SunMarc Oct 18, 2024

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

@gante
Copy link
Member Author

gante commented Oct 29, 2024

I hope you meant warning, not exception

@zucchini-nlp haha yes, warning! In essence, allowing the behavior.

@gante
Copy link
Member Author

gante commented Oct 29, 2024

@zucchini-nlp @SunMarc PR comments addressed, LMK if they look good to you :)

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

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