-
Notifications
You must be signed in to change notification settings - Fork 655
feat: b64 encoding for nixl_connect metadata #3843
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Alexandre Milesi <[email protected]>
WalkthroughThe pull request modifies internal encoding mechanisms for NIXL metadata in the Python RDMA connection module, switching serialization from hexadecimal to base64 encoding in RdmaMetadata construction and Remote initialization paths. No public API changes were introduced. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This PR makes sense and looks good to me. But @whoisj should take a look as well |
| # representation of the NIXL metadata. | ||
| if isinstance(nixl_metadata, str): | ||
| # Decode the hex-encoded string into bytes. | ||
| nixl_metadata = bytes.fromhex(nixl_metadata) |
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.
while b64 encoding is a win in size, we cannot just change the encoding and walk away.
please add a method to fallback to hex encoding to retain compatability.
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.
Added a fallback to hex in case people work with mismatched nixl_connect versions.
In some cases b64 and hex data cannot be distinguished so I added a small identifiable prefix for the new b64 encoding.
Signed-off-by: Alexandre Milesi <[email protected]>
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.
code changes LGTM, but has this been tested using the multimodal examples?
Overview:
Changes the encoding of the compressed NIXL agent metadata
Details:
hexencoding has an overhead of 100%, doubling the size of the actual data. This means that if the zlib compression ratio is less than 2x (which it seems to be in many cases), then the metadata send over the network after compression+encoding is even larger than the original metadata size.Base64 has a 33% overhead and mitigates this while keeping the payload JSON-safe. Base85 could be another option with 25% overhead but I feel like base64 has more support for external integration and compatibility.
Example:
@whoisj
Summary by CodeRabbit