Skip to content

[dynamo] Clean up assert in dynamo [3/N] #165903

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

[dynamo] Clean up assert in dynamo [3/N] #165903

wants to merge 7 commits into from
+65 −7

Conversation

@can-gaa-hou
Copy link
Contributor

@can-gaa-hou can-gaa-hou commented Oct 20, 2025 •
edited by pytorch-bot bot
Loading

Some previous PRs have been merged. This PR aims for some assert that the users can trigger, and it may be better to turn them into a graph break. Correct me if there are any problems.

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames @Lucaskabela @williamwen42

@pytorch-bot
Copy link

pytorch-bot bot commented Oct 20, 2025 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/165903

Note: Links to docs will display an error until the docs builds have been completed.

✅ You can merge normally! (1 Unrelated Failure)

As of commit 6c31dc3 with merge base 32fe4f6 (image):

FLAKY - The following job failed but was likely due to flakiness present on trunk:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@can-gaa-hou can-gaa-hou mentioned this pull request Oct 20, 2025
Closed
pytorchmergebot pushed a commit that referenced this pull request Oct 22, 2025
@can-gaa-hou @pytorchmergebot
[dynamo] Clean up assert in dynamo [2/N] (#165745)
a479769 
Extend from #165430
* #165903(Clean up for graph break)
* ->#165745
* #165430

One main refractor from the previous PR:
* For assertions like checking `len(args)` or `len(kwargs)`, using `raise_args_mismatch` instead of `raise_type_error_exc`

I am also considering moving `raise_type_error_exc` into `utils.py` for consistency.

Pull Request resolved: #165745
Approved by: https://github.com/Lucaskabela
@can-gaa-hou
Copy link
Contributor Author

@pytorchbot label "topic: not user facing"

@pytorch-bot pytorch-bot bot added the topic: not user facing topic category label Oct 24, 2025
@can-gaa-hou can-gaa-hou marked this pull request as ready for review October 24, 2025 09:47
williamwen42
williamwen42 reviewed Oct 27, 2025
View reviewed changes
torch/_dynamo/variables/ctx_manager.py Outdated
@staticmethod
def create(func, args, kwargs):
assert func in [
if func not in [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assert is fine to leave as-is since we should only be creating this variable if the condition is true from the caller. (code pointer

pytorch/torch/_dynamo/variables/torch.py

Line 398 in 9901d44

return AutocastModeVariable.create(self.value, args, kwargs)
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion :) I have updated it.

torch/_dynamo/variables/misc.py Outdated
"queue_callback() is only supported when Compiled Autograd is enabled with fullgraph=True"
)
if not (tx.one_graph or tx.error_on_graph_break):
unimplemented_v2(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xmfan is it ok to graph break here? or should we error out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like there has been a change here recently. Let's keep that first.

can-gaa-hou and others added 7 commits October 28, 2025 20:36
@can-gaa-hou
Clean up assert in dynamo
60312f5
@can-gaa-hou @williamwen42
Apply suggestion from @williamwen42
7d307ec 
Co-authored-by: William Wen <william.wen42@gmail.com>
@can-gaa-hou @williamwen42
Update torch/_dynamo/variables/distributed.py
e44b666 
Co-authored-by: William Wen <william.wen42@gmail.com>
@can-gaa-hou @williamwen42
Update torch/_dynamo/variables/misc.py
1d5b40a 
Co-authored-by: William Wen <william.wen42@gmail.com>
@can-gaa-hou @williamwen42
Update torch/_dynamo/variables/misc.py
d802bf6 
Co-authored-by: William Wen <william.wen42@gmail.com>
@can-gaa-hou @williamwen42
Update torch/_dynamo/variables/misc.py
9d57ad3 
Co-authored-by: William Wen <william.wen42@gmail.com>
@can-gaa-hou
Fix some issues in comment
6c31dc3
@soulitzer soulitzer added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Oct 28, 2025
@williamwen42
Copy link
Member

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 28, 2025
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@williamwen42 williamwen42 williamwen42 approved these changes

Assignees

No one assigned

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged module: dynamo open source topic: not user facing topic category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@can-gaa-hou @williamwen42 @pytorchmergebot @soulitzer @pytorchbot