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

Merged
merged 7 commits into from
Nov 15, 2024
Merged

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

@gante gante requested review from SunMarc and zucchini-nlp October 18, 2024 10:40
Comment on lines -400 to -413
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 -413
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 gante force-pushed the deprecations_v4_44 branch from dcd2588 to 48f5982 Compare October 29, 2024 17:26
@gante gante requested review from SunMarc and zucchini-nlp October 29, 2024 17:27
@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.

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, looks good to me! Can we propagate the same thing to all files with this deprecation warning? I guess it is all LLaVA and some of BLIP models (processing and modeling files)

@gante
Copy link
Member Author

gante commented Oct 30, 2024

@zucchini-nlp done ✅

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.

Nice ! Thanks for iterating !

@SunMarc SunMarc requested a review from ArthurZucker November 4, 2024 15:54
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.

Ok, sounds good! Thanks for the PR and cleanup @gante

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.

🧼 thanks

@SunMarc
Copy link
Member

SunMarc commented Nov 5, 2024

To pass the CI, you need to rebase the PR @gante ;)

@ArthurZucker ArthurZucker merged commit 1349321 into huggingface:main Nov 15, 2024
24 of 26 checks passed
2015aroras pushed a commit to 2015aroras/transformers that referenced this pull request Nov 15, 2024
* remove v4.44 deprecations

* PR comments

* deprecations scheduled for v4.50

* hub version update

* make fiuxp

---------

Co-authored-by: Marc Sun <[email protected]>
Co-authored-by: Arthur <[email protected]>
BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
* remove v4.44 deprecations

* PR comments

* deprecations scheduled for v4.50

* hub version update

* make fiuxp

---------

Co-authored-by: Marc Sun <[email protected]>
Co-authored-by: Arthur <[email protected]>
BernardZach pushed a commit to innovationcore/transformers that referenced this pull request Dec 6, 2024
* remove v4.44 deprecations

* PR comments

* deprecations scheduled for v4.50

* hub version update

* make fiuxp

---------

Co-authored-by: Marc Sun <[email protected]>
Co-authored-by: Arthur <[email protected]>
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.

6 participants