-
Notifications
You must be signed in to change notification settings - Fork 182
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
Container Streaming/Retriever #3173
Conversation
/build |
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.
Looks good in general. See my comments for enhancement.
A general comment: shall we do a comparison of memory footprint standard v.s. dict streaming v.s. file streaming? I will do some experiment on how to track the memory usage, I think using JobAPI should work |
I did some experiments but it seems the basic transmission without dict streaming has similar memory footprint as compared with streaming, not sure if it is expected (@nvidianz I created a PR on your branch, let me know if you find any mistakes in my code) |
Mis-interpreted the results last Friday, the result actually makes sense: Performed a test with a 6 GB model: Considering the largest layer is ~1GB, the difference should be ~5GB, so the above results on diff are reasonable, note that the recorded memory usage is for the general system, so only the diff is meaningful |
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.
looks good and I can confirm the functionality part. This PR is ready for merge once Yan's comment is addressed, I will created a separate PR to refine the example.
/build |
0bbc352
to
344d606
Compare
/build |
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
Description
Types of changes
./runtest.sh
.