-
Notifications
You must be signed in to change notification settings - Fork 287
Support using paddle in safe_open #630
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
Conversation
yuanlehome
left a comment
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.
LGTM,We hope that official classmates can help review and promote code integration. Thanks!
Narsil
left a comment
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.
Looking good left a few comments.
bindings/python/src/lib.rs
Outdated
| "paddle" => Ok(Framework::Paddle), | ||
| "paddlepaddle" => Ok(Framework::Paddle), | ||
| "pp" => Ok(Framework::Paddle), |
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.
Can we remove paddlepaddle (I don't see any reference anywhere in the tutorials).
Same for pp unless I missed it as something being regularly used.
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.
We can remove paddlepaddle. But we should save pp. pp in paddle just like pt in pytorch and np in numpy.
I also use pp in test case file. Do you mean that the abbreviation is not required?
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.
I meant, can you point me to places where the abbreviation is used "naturally". Something like tutorials, examples docs from the official paddlepaddle project (or some big user of it).
For numpy (np): https://numpy.org/doc/stable/user/absolute_beginners.html
For pytorch: https://huggingface.co/docs/transformers/main_classes/tokenizer#transformers.PreTrainedTokenizerFast.__call__.return_tensors It's a convention used in a few key places in HF's ecosystem hence why I went with it (despite my preference for having only 1 clear name for things).
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.
Right, it seems no need to save pp, I will remove it later.
bindings/python/src/lib.rs
Outdated
| let version = Version::from_string(&version).map_err(SafetensorError::new_err)?; | ||
|
|
||
| // todo: version check, only paddle 3.1 or develop | ||
| if version >= Version::new(3, 1, 0) || version >= Version::new(0, 0, 0) { |
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.
Can you remove version 0.0.0 ?
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.
Version 0.0.0 means user uses the develop version. The latest develop version have supported this function. Maybe I should use == but not >=.
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.
Then yes == is preferred, otherwise afaik users for versions > 0.0.0 < 3.1.0 will encounter issues, right ?
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.
Yes, this feature only support for paddle>=3.1.1 and develop version(0.0.0). It should use the Mmap of safetensors for versions > 0.0.0 < 3.1.1.
bindings/python/src/lib.rs
Outdated
| if cur_type == Dtype::U16 { | ||
| // paddle set bf16 as u16 | ||
| cur_type = Dtype::BF16; | ||
| } |
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.
info is the source in the safetensors file, I think this is reversed logic.
Try to use non mutable variables. let cur_type = if info.dtype == xxx{ something } else{ something else};
Make it much easier to be sure no one if modifying this later on.
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.
Ok, I will modify it.
| @@ -1,7 +1,10 @@ | |||
| import threading | |||
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.
Remove this import.
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.
Ok
| "a": A, | ||
| } | ||
| ident = threading.get_ident() | ||
| save_file(tensors, f"./tensor_{ident}.safetensors") |
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.
Just use a unique name, no need to use ident.
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.
Ok
bindings/python/src/lib.rs
Outdated
| enum Device { | ||
| Cpu, | ||
| Cuda(usize), | ||
| Gpu(usize), |
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.
No, it's called CUDA, let's not add something new. Let's reuse cuda(usize). If paddle uses a different name, let's just make sure the conversion happen correctly when they are called (so in paddle only code).
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.
ok, I will remove it.
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.
Re-reading this I have question about GPU semantics with paddle, how does it deal with non cuda GPUs ?
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.
Sorry, I misunderstood your question. The gpu in paddle refers to the cuda GPU, and other non cuda GPUs are represented by xxx_gpu(like intel_gpu/metax_gpu). This repo will show more ways to access hardware. https://github.com/PaddlePaddle/PaddleCustomDevice
|
I have modified the code following the comments. Could you review it again? Thanks! @Narsil |
|
Since https://github.com/huggingface/safetensors/pull/646/commits is open, maybe we should close this ? It seems the overlap is significant here, right ? |
No, this PR #646 still has some content that needs to be modified, but the |
|
Well my comments from here still apply. This PR is breaking paddle <3.1.1 |
OK, I am install the paddlepaddle == 3.0.0. I run the test file I should handle the device when I use Is there any other testing case which will trigger the breaking when paddle < 3.1.1 ? |
Narsil
left a comment
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.
LGTM.
|
We can remove the Storage suffix for both variants to make clippy happy. |
|
Actually let me merge it. This is not important. |

Support using paddle in
safe_openAPIThis PR allows us to use
safe_openAPI when set theframework='pp'orframework='paddle'. It just like the following code:In this PR, I use the latest feature(MmapStorage) in paddle. It only support for paddle >= 3.1.1 or develop version.
We also can use the original way to load tensor if the paddle version is lower than 3.1.1.