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

Use huggingface_hub helper function to split state dict #31091

Merged
merged 7 commits into from
Jun 12, 2024

Conversation

SunMarc
Copy link
Member

@SunMarc SunMarc commented May 28, 2024

What does this PR do ?

This PR uses the helper function from huggingface-hub to split a state dict into shards instead of shard_checkpoint. This will make maintenance easier for all HF libraries. Similar PR have been created in accelerate and diffusers.

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

This looks good in theory to me; @Wauplin, could you give this a quick look as well?

@SunMarc
Copy link
Member Author

SunMarc commented May 31, 2024

Could you make get_storage_id public/documented since it is used in multiple places (not only when cleaning the state dict) @Wauplin ? Thanks !

Copy link
Contributor

@Wauplin Wauplin left a comment

Choose a reason for hiding this comment

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

Looks good to me as well! Thanks for taking care of this @SunMarc :)

If I'm not mistaken there are similar methods for tf / flax in transformers that could benefit from it. Can be done in separate PRs though.

@SunMarc
Copy link
Member Author

SunMarc commented May 31, 2024

If I'm not mistaken there are similar methods for tf / flax in transformers that could benefit from it. Can be done in separate PRs though.

I'll have a look and do it in another PR after this PR gets merged and tested !

Just a quick question @Wauplin , the current helper function in huggingface-hub introduced a regression in transformers (not able to handle kiB, miB). Is is fine ? If so, I can modify the failing test : test_checkpoint_sharding_local_bin

@Wauplin
Copy link
Contributor

Wauplin commented May 31, 2024

Could you make get_storage_id public/documented since it is used in multiple places (not only when cleaning the state dict) @Wauplin ?

@SunMarc Done in huggingface/huggingface_hub#2304

he current helper function in huggingface-hub introduced a regression in transformers (not able to handle kiB, miB). Is is fine ?

I'm not a transformers's maintainer but I would assume it's fine. I'm quite convinced those parameters are never used (expect for the default value) and even less using kib, mib, etc. notation. If others feel strongly about it, we could somehow mitigate it directly in transformers (typically with a max_size.lower().replace("mib", "mb").replace("gib", "gb") + a deprecation warning). But I'd prefer not to update huggingface_hub for those.

@SunMarc SunMarc requested a review from amyeroberts June 10, 2024 15:38
@SunMarc
Copy link
Member Author

SunMarc commented Jun 10, 2024

I'm not a transformers's maintainer but I would assume it's fine. I'm quite convinced those parameters are never used (expect for the default value) and even less using kib, mib, etc. notation. If others feel strongly about it, we could somehow mitigate it directly in transformers (typically with a max_size.lower().replace("mib", "mb").replace("gib", "gb") + a deprecation warning). But I'd prefer not to update huggingface_hub for those.

WDYT @amyeroberts ?

@amyeroberts
Copy link
Collaborator

but I would assume it's fine. I'm quite convinced those parameters are never used (expect for the default value) and even less using kib, mib, etc. notation. If others feel strongly about it, we could somehow mitigate it directly in transformers (typically with a max_size.lower().replace("mib", "mb").replace("gib", "gb") + a deprecation warning). But I'd prefer not to update huggingface_hub for those.

@Wauplin @SunMarc Sounds good to me! Agreed, I don't think we need to handle a deprecation cycle here

@SunMarc SunMarc merged commit 254b25a into main Jun 12, 2024
21 checks passed
@SunMarc SunMarc deleted the shard_saving_from_hf_hub branch June 12, 2024 12:10
zucchini-nlp pushed a commit to zucchini-nlp/transformers that referenced this pull request Jun 14, 2024
…31091)

* shard saving from hf hub

* index = None

* fix tests

* indent
itazap pushed a commit that referenced this pull request Jun 17, 2024
* shard saving from hf hub

* index = None

* fix tests

* indent
itazap pushed a commit that referenced this pull request Jun 17, 2024
* shard saving from hf hub

* index = None

* fix tests

* indent
itazap pushed a commit that referenced this pull request Jun 17, 2024
* shard saving from hf hub

* index = None

* fix tests

* indent
itazap pushed a commit that referenced this pull request Jun 18, 2024
* shard saving from hf hub

* index = None

* fix tests

* indent
itazap pushed a commit that referenced this pull request Jun 20, 2024
* shard saving from hf hub

* index = None

* fix tests

* indent
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.

5 participants