-
Notifications
You must be signed in to change notification settings - Fork 224
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
Safetensors #1255
Safetensors #1255
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchchat/1255
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 9db974d with merge base d8c0aaf (): This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Hi @gabe-l-hart! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
NOTE: The CLA is in process since I'm contributing through the IBM corporate CLA. |
Thanks @gabe-l-hart |
Thanks @byjlw! I'll get the rebase done shortly. As you noted, this is part of my work to add Granite Code support. I have a single branch pointer for all that work, but I have also tried to organize the commits so that they can be merged as bite-sized features rather than a big overhaul. Right now, the addition of the configs for Granite Code are at the end of the chain since the model won't work at all until the other features are present (it also currently only works in the python runtime). I'm totally open to whatever convention the |
8a00032
to
625c12c
Compare
chunks are definitely better. Would love to learn more about the overall goals for the Granite code support |
Good point, I didn't ever spell this out! Let me open a top-level issue about that support that I can use to track it all. |
Top-level Granite Code support issue: #1262 |
Actually though, when i tested it by changing the the model.json to use safetensors for 11b base download errored out. It looks like this code works as long as it's not using definitions that use the torchtune format. Do you mind testing this case and resolving the issue?
|
Ah, good catch! I guess |
@ebsmothers can also help |
Great. I'll report progress or blockers as they come up. @ebsmothers let me know if you dig in and get anywhere! |
@byjlw so I'm not super familiar with how torchchat is handling checkpoint conversion but if you're switching from the .pth format to .safetensors format you will no longer be able to just do the simple move that's happening here. The safetensors format for Llama 3.2 11B Vision distributes the model weights across multiple files (see here) so they will need to be loaded and merged into a single state dict as we do in torchtune here. |
That makes sense. A similar approach is being taken in convert_hf_checkpoint to load the sharded weights and convert them into the I finally have the safetensors downloaded locally, so I'll see how far I can get with this approach. |
I have the mechanics of this working for the Given that, and given the fact that these models do already have checkpoints that match the target naming scheme, I think it might make sense to leave the PR as-is for now and not switch |
625c12c
to
2fc163c
Compare
…r names Branch: GraniteCodeSupport Signed-off-by: Gabe Goodhart <ghart@us.ibm.com>
Branch: GraniteCodeSupport Signed-off-by: Gabe Goodhart <ghart@us.ibm.com>
…h or safetensors The logic here will prefer pth over safetensors unless the model's config explicitly states a preference for safetensors over pth. If only one of the two is found, the download will use whichever is present. Branch: GraniteCodeSupport Signed-off-by: Gabe Goodhart <ghart@us.ibm.com>
2fc163c
to
9db974d
Compare
@@ -41,7 +42,12 @@ def convert_hf_checkpoint( | |||
print(f"Model config {config.__dict__}") | |||
|
|||
# Load the json file containing weight mapping | |||
model_map_json = model_dir / "pytorch_model.bin.index.json" | |||
model_map_json_matches = [Path(m) for m in glob.glob(str(model_dir / "*.index.json"))] | |||
assert len(model_map_json_matches) <= 1, "Found multiple weight mapping files" |
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.
Why is this an error? Thanks!
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.
Good catch. See my response over on your PR: #1346 (comment)
Description
Closes #1249
This PR implements support for downloading and converting model checkpoints from
huggingface
which use thesafetensors
format rather than thepth
binary (pickle) format.Changes
*.index.json
namessafetensors.torch.load
when neededTesting
llama3.1
are un-changed with these changes (nosafetensors
are downloaded)safetensors
, they are not ignored and can be cleanly converted