-
Notifications
You must be signed in to change notification settings - Fork 108
Move DTensor isinstance check from runtime to trace construction time in nvFuser executor #2652
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
Co-authored-by: IvanYashchuk <[email protected]>
Co-authored-by: IvanYashchuk <[email protected]>
|
@copilot, the changes look good. Please add a test for this change. |
Co-authored-by: IvanYashchuk <[email protected]>
Added tests in commit 2c67caa:
Both tests check that the |
IvanYashchuk
left a comment
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.
That's exactly what I wanted.
|
@kshitij12345, do you agree with the change? |
|
|
||
|
|
||
| @common_utils.parametrize("executor", tuple(executors_map.keys())) | ||
| def test_use_dtensor_execute_flag_with_dtensor(self, executor): |
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.
@copilot, This test should run only for nvfuser executor otherwise assert len(fusions) > 0 will be false
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.
@copilot, apply the suggestion above.
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.
Applied the suggestion in commit 0ed6e23. The test now skips for non-nvfuser executors to avoid assertion failures when there are no fusions.
Co-authored-by: IvanYashchuk <[email protected]>
|
@copilot test test are you active? |
for more information, see https://pre-commit.ci
Plan to Fix DTensor isinstance Check Issue
use_dtensor_executeattribute toFusionDefinitionWrapperclassuse_dtensor_executevalue at trace construction time increate_fusion_definition_wrapperFusionDefinitionWrapper.__call__to useuse_dtensor_executeinstead of runtime isinstance checkSummary
Successfully moved the DTensor type check from runtime to trace construction time in the nvFuser executor. The changes ensure that the dispatch to DTensor-based execution is determined by the proxy types in the trace rather than by checking runtime arguments.
Key Changes
Added
use_dtensor_executefield toFusionDefinitionWrapper(line 537):Falsefor backward compatibilityCompute
use_dtensor_executeat trace construction (line 623):create_fusion_definition_wrapper, check if anysorted_unique_inputsis aDTensorProxyUse pre-computed flag instead of runtime check (line 556):
if dist.is_available() and any(isinstance(t, torch.distributed.tensor.DTensor) for t in args)if self.use_dtensor_executeFixed guard for distributed availability (line 548):
if dist.is_available()if IS_TORCH_DISTRIBUTED_AVAILABLENameErrorwhentorch.distributedis not availableTests Added
test_use_dtensor_execute_flagintest_nvfuser.py:use_dtensor_executeisFalsefor regular tensor inputstest_use_dtensor_execute_flag_with_dtensorintest_dtensor.py:use_dtensor_executeisTruefor DTensor inputsBenefits
Original prompt
Fixes #2651
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.