🚨🚨 [saving] Default to 50GB shards, and remove non-safe serialization - #42734
Conversation
|
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. |
ArthurZucker
left a comment
There was a problem hiding this comment.
LGTM put a lot of 🔴 on top please
|
Spotted a few places where
Would be great to entirely get rid of them |
|
Yep, and much more in the tests 🥲 Should have removed them all now |
SunMarc
left a comment
There was a problem hiding this comment.
Thanks ! Left some minor comments !
| return dtype | ||
|
|
||
| def get_state_dict_and_metadata(self, model, safe_serialization: bool | None = False): | ||
| def get_state_dict_and_metadata(self, model): |
There was a problem hiding this comment.
cc @jerryzh168 we will remove bin serialization from now on.
|
Thanks @Cyrilvallez for the update. I've pushed bf8680d as well to update the musicgen docs. Otherwise the torchao docs are mentioning:
What should be done with this since |
|
@Wauplin the doc is wrong here, it does support it since version > 0.15.0 |
…mers into increase-shard-size
(I'll let you update it, I'm not exactly sure what should be written) |
| # (state_dict tensors are detached and therefore no longer shared) | ||
| tensor = self.get_parameter(name) | ||
| ptrs[id(tensor)].append(name) | ||
| # Safetensors does not allow tensor aliasing - we're going to remove aliases before saving |
There was a problem hiding this comment.
chatted with @ArthurZucker, we might update safetensors to handle aliasing better than the current state of affairs.
will ping when I have more on that!
|
[For maintainers] Suggested jobs to run (before merge) run-slow: aimv2, aria, chameleon, csm, deepseek_vl, deepseek_vl_hybrid, depth_pro, efficientloftr, emu3, eomt, fuyu, gemma, gemma2, gemma3, gemma3n, gpt_oss |
|
View the CircleCI Test Summary for this PR: https://huggingface.co/spaces/transformers-community/circle-ci-viz?pr=42734&sha=f3ea9e |
…huggingface#42734) * switch * remove now useless save_function * a bit more involved than i thought * all converters * fix * pretty print * fix * trainer * update musicgen.md docs * marc comments * doc and last missed instances * CI --------- Co-authored-by: Wauplin <lucainp@gmail.com> Co-authored-by: Arthur <48595927+ArthurZucker@users.noreply.github.com>
What does this PR do?
As per the title.
This PR fixes both feature requests #42555 and #42556.
Note that increasing the shard size was a decision that was only taken after very careful considerations, and the following benchmarks:
Benchmarking scripts
First, the following script was used to create model checkpoints of several sizes and several shard sizes:
create_models.py
Then the following benchmarking script was run to obtain the data:
benchmark.py
Results
TLDR: we don't find any significative impact on loading time coming from the shard size (in different scenarios, i.e. all 4 combinations of cpu/gpu loading and native/new dtype), so we are free to increase to the now much better 50GB shard size recommendation coming from the hub.
Here, we present the results obtained. Note that all the models were saved in
bf16(torch.bfloat16), so loading them back in any other dtype requires an additional copy of the data, which is expected to take longer!We always present main results when the checkpoints are hot in ssd, as well as timings during warmup loading runs. Note that this is not always perfectly reliable as we absolutely cannot control how the OS will cache the files, but 4 warmup loading runs should be enough. We still notice bigger errors bars on main runs for a few datapoints, meaning that the checkpoints were not perfectly cached yet.
Note: these results were obtained on a node of our cluster as of December 2025, i.e. a node with 8xH100 GPUs, and 2TiB cpu RAM.
device_map="auto",dtype=torch.bfloat16(native dtype)device_map="cpu",dtype=torch.bfloat16(native dtype)device_map="auto",dtype=torch.float16(NON-native dtype, needs copy)device_map="cpu",dtype=torch.float16(NON-native dtype, needs copy)(not sure what happened for 120B on this one, looks like the cpu got saturated a bit - but the trend is the same anyway)