-
-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Multi Modal] Configurable MM Profiling #25631
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
This pull request has merge conflicts that must be resolved before it can be |
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 Review
This pull request introduces a valuable feature for making multimodal profiling more configurable, allowing for more precise memory allocation. The changes, which include new dataclasses for modality options and updated configuration parsing, are well-structured and maintain backward compatibility. However, I've identified a critical issue with the use of overly broad exception handling in the profiling logic. This could mask underlying bugs and lead to incorrect memory profiling, potentially causing runtime errors. My review includes specific suggestions to address this by narrowing the exception scope and improving debuggability.
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.
Thanks for working on this, can you fix the merge conflicts?
2df6898
to
85c6947
Compare
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Feel free to update each model now |
Signed-off-by: wwl2755 <wangwenlong2755@gmail.com>
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: wwl2755 <wangwenlong2755@gmail.com>
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.
/gemini review
Thanks, I'm waiting for upstream CI to be fixed before adding |
Can you merge from main again to get the CI running? |
Signed-off-by: wwl2755 <wangwenlong2755@gmail.com>
Head branch was pushed to by a user without write access
Signed-off-by: wwl2755 <wangwenlong2755@gmail.com>
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.
Tests pass so LGTM, thanks!
Signed-off-by: wwl2755 <wangwenlong2755@gmail.com> Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Co-authored-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Signed-off-by: Rahul Tuli <rtuli@redhat.com>
Signed-off-by: wwl2755 <wangwenlong2755@gmail.com> Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Co-authored-by: Harry Mellor <19981378+hmellor@users.noreply.github.com> Signed-off-by: yewentao256 <zhyanwentao@126.com>
Fix #14438
LAST EDIT: 09/26
image
, we can specifycount
,width
,height
.For
video
, we can sepcifycount
,width
,height
,num_frames
.For
audio
, we can specifycount
,length
.Known issues/TODOs:
Test
Tested on A100-40GB