-
-
Notifications
You must be signed in to change notification settings - Fork 10.7k
[BugFix] fix graph partition signature #27139
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
[BugFix] fix graph partition signature #27139
Conversation
Signed-off-by: Boyuan Feng <[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 Review
This pull request introduces a monkey patch for get_graph_partition_signature in PyTorch 2.9 to address a bug related to graph partitioning, which fixes the test_attn_quant test. The changes in the test file reflect an update to the logging infrastructure. The core change in vllm/env_override.py seems correct in its intent to backport a fix. However, there is a misleading comment for the new monkey patch that should be corrected to improve maintainability.
tests/compile/test_fusions_e2e.py
Outdated
| ) | ||
|
|
||
| with caplog_mp_spawn(logging.DEBUG) as log_holder: | ||
| with caplog_vllm.at_level(logging.DEBUG): |
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.
This test does not use mp. caplog_mp_spawn would also fail for dynamo graph partition.
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 wonder if that's just on your machine - for me even tp=1 tests used MP. I'll enable CI and we can see. But I think we should probably just do both, and we can investigate later
Signed-off-by: Boyuan Feng <[email protected]>
tests/compile/test_fusions_e2e.py
Outdated
| ) | ||
|
|
||
| with caplog_mp_spawn(logging.DEBUG) as log_holder: | ||
| with caplog_vllm.at_level(logging.DEBUG): |
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 wonder if that's just on your machine - for me even tp=1 tests used MP. I'll enable CI and we can see. But I think we should probably just do both, and we can investigate later
Signed-off-by: Boyuan Feng <[email protected]>
|
@ProExpertProg yes we should use |
Signed-off-by: Boyuan Feng <[email protected]>
Signed-off-by: Boyuan Feng <[email protected]>
A pytorch 2.9 monkey patch to fix
test_attn_quant. See pytorch/pytorch#165815