Introduce a Copier interface to support custom scenarios #32
Conversation
ABNER-1
changed the title
ABNER-1
force-pushed
the
custom_reader
branch
3 times, most recently
from
10b1d74 to
40a5d58
Compare
There was a problem hiding this comment.
Thank you for contributing the nice code for CopierInterface!
Maybe this is a discussion point, but I don't think new_gds_file_copier and splitting SafeTensorsFileLoader are necessary at this moment. I understand they makes the code cleaner, but I am a bit anxious that they will gradually cause the difficulty to follow where the object came from with future changes. so, could you please keep the copier construction and the class definition like before if it is possible.
| pass | ||
|
|
||
|
|
||
| class DummyDeviceBuffer(fstcpp.gds_device_buffer): |
There was a problem hiding this comment.
Looks new codes do not use this class. Is this required for your future changes? what kind of code do you expect to use it?
There was a problem hiding this comment.
I will implement an example copier to handle GPU memory allocation within the reader.
This is managed in C++ code, which releases python GIL, thus improving overall performance.
The DummyDeviceBuffer can help me seamlessly integrates into the framework's workflow.
There was a problem hiding this comment.
I see. Can you move this change to the future change?
|
|
||
| class SafeTensorsFileLoader: | ||
| r"""Load .safetensors files lazily. | ||
| class BaseSafeTensorsFileLoader: |
There was a problem hiding this comment.
Do we need to split the class into two? Looks BaseSafeTensorsFileLoader is not reusable anywhere.
There was a problem hiding this comment.
This approach provides a clean way to implement a new copier and loader without modifying any code in the BaseSafeTensorsFileLoader, while also ensuring the existing interfaces in fastsafetensors remain unchanged.
|
@ABNER-1 |
Apologies for the delay; I was tied up with other tasks. I'm back on this now and will address all feedback this week. |
ABNER-1
force-pushed
the
custom_reader
branch
from
40a5d58 to
33603c9
Compare
Signed-off-by: yuanyuxing.yyx <[email protected]>
ABNER-1
force-pushed
the
custom_reader
branch
from
33603c9 to
1f0924e
Compare
|
Hi, @takeshi-yoshimura . A well-designed copier interface allows us to leverage the two-phase loading and broadcasting mechanism of fastsafetensors while minimizing changes to existing abstractions and code. In practice, different companies often utilize internal network storage protocols with highly customized loading SDKs (e.g., leveraging RDMA or GDR). With this interface, they would only need to implement a custom copier to integrate seamlessly. This significantly expands the applicability of fastsafetensors. I hope this proposal can gain your approval. |
|
Thanks, let me see the change closely tomorrow. |
takeshi-yoshimura
merged commit e99b606
into
foundation-model-stack:main
|
Merged. Thanks. |
2 participants
The 3rd in #29
We recommend abstracting the Copier to allow easier adaptation for more scenarios. For instance, we have implemented a custom Copier for network storage scenarios, which can be highly optimized for specific use cases.