Skip to content

Automatically turn on dynamo in cond #108028

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 13 commits into from
Closed

Automatically turn on dynamo in cond #108028

wants to merge 13 commits into from

Conversation

ydwu4
Copy link
Contributor

@ydwu4 ydwu4 commented Aug 27, 2023 •
edited
Loading

@ydwu4 ydwu4 mentioned this pull request Aug 27, 2023
Closed
@pytorch-bot
Copy link

pytorch-bot bot commented Aug 27, 2023 •
edited
Loading

🔗 Helpful Links

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

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

✅ No Failures

As of commit 728b269 with merge base 1fd4e78 (image):
💚 Looks good so far! There are no failures yet. 💚

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

@ydwu4 ydwu4 mentioned this pull request Aug 27, 2023
Closed
@pytorch-bot pytorch-bot bot added the release notes: fx release notes category label Aug 27, 2023
@ydwu4 ydwu4 mentioned this pull request Aug 27, 2023
Closed
@ydwu4
Update on "Automatically turn on dynamo in cond"
87d7246 
cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx chenyang78 aakhundov

[ghstack-poisoned]
@ydwu4
Update on "Automatically turn on dynamo in cond"
12f16c3 
cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx chenyang78 aakhundov

[ghstack-poisoned]
@ydwu4 ydwu4 mentioned this pull request Aug 27, 2023
Closed
@ydwu4 ydwu4 requested a review from zou3519 August 27, 2023 21:52
zou3519
zou3519 reviewed Aug 28, 2023
View reviewed changes
zou3519
zou3519 reviewed Aug 28, 2023
View reviewed changes
torch/_higher_order_ops/cond.py
Comment on lines +39 to +41
# We need to turn off the is_fx_tracing_flag. Remove this flag check from dyanmo
# once we are confident fx tracing works with dynamo.
torch.fx._symbolic_trace._is_fx_tracing_flag = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I still don't understand why we are turning this flag to False. You mentioned that making it go to True would make a lot of tests fail. But we shouldn't be regressing behavior, unless they're related to the symbool thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make_fx will 1. turn this flag on and 2. feed FakeTensors and SymBools into torch.compile. I kind of assume this flag can be turned off in cond_compiled and it's not a problem.

Right now, if we leave this flag alone, we'll get an error for all make_fx tests, which isn't what we want we already supports "real" tracing mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the problem that in torch.compile, we want _is_fx_tracing_flag to be False? If so, I would have expected that we turned that off inside of torch.compile, because this is a make_fx and torch.compile composability issue, rather than doing a one-off solution here

zou3519
zou3519 reviewed Aug 28, 2023
View reviewed changes
zou3519
zou3519 reviewed Aug 28, 2023
View reviewed changes
zou3519
zou3519 reviewed Aug 28, 2023
View reviewed changes
zou3519
zou3519 reviewed Aug 28, 2023
View reviewed changes
zou3519
zou3519 reviewed Aug 28, 2023
View reviewed changes
zou3519
zou3519 reviewed Aug 28, 2023
View reviewed changes
zou3519
zou3519 reviewed Aug 28, 2023
View reviewed changes
test/dynamo/test_export.py

self.assertTrue(torch.allclose(m(x), gm(x)))

@unittest.expectedFailure
Copy link
Contributor

Choose a reason for hiding this comment

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

What's going on here? IIRC this test is important for quantization for 2.1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have a check and get back. It has some complicated errors as I remember.

ydwu4 added 4 commits August 27, 2023 18:20
@ydwu4
Update on "Automatically turn on dynamo in cond"
2ca5934 
A replacement of #107932.

cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx chenyang78 aakhundov

[ghstack-poisoned]
@ydwu4
Update on "Automatically turn on dynamo in cond"
6ba7e9a 
A replacement of #107932.

cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx chenyang78 aakhundov

[ghstack-poisoned]
@ydwu4
Update on "Automatically turn on dynamo in cond"
3ccb577 
A replacement of #107932.

cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx chenyang78 aakhundov

[ghstack-poisoned]
@ydwu4
Update on "Automatically turn on dynamo in cond"
51e7e02 
A replacement of #107932.

cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx chenyang78 aakhundov

[ghstack-poisoned]
zou3519
zou3519 reviewed Aug 28, 2023
View reviewed changes
@ydwu4
Copy link
Contributor Author

ydwu4 commented Aug 28, 2023

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Aug 28, 2023
@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

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 1 jobs have failed, first few of them are: trunk / macos-12-py3-arm64 / test (default, 3, 3, macos-m1-12)

Details for Dev Infra team Raised by workflow job

@ydwu4
Update on "Automatically turn on dynamo in cond"
1b9366b 
A replacement of #107932.

cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx chenyang78 aakhundov

[ghstack-poisoned]
@ydwu4
Copy link
Contributor Author

ydwu4 commented Aug 28, 2023

@pytorchbot merge

@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

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 2 mandatory check(s) failed. The first few are:

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

Failing merge rule: Core Maintainers

@ydwu4
Update on "Automatically turn on dynamo in cond"
728b269 
A replacement of #107932.

cc voznesenskym penguinwu anijain2305 EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx chenyang78 aakhundov

[ghstack-poisoned]
ydwu4 added a commit that referenced this pull request Aug 28, 2023
@ydwu4
Automatically turn on dynamo in cond
6a310b5 
ghstack-source-id: d09e209
Pull Request resolved: #108028
@ydwu4
Copy link
Contributor Author

ydwu4 commented Aug 28, 2023

@pytorchbot merge

@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

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: Command git -C /home/runner/work/pytorch/pytorch cherry-pick -x 6a310b56942568d84f8f3b4b831aae0190f54e11 returned non-zero exit code 1

Auto-merging test/dynamo/test_export.py
CONFLICT (content): Merge conflict in test/dynamo/test_export.py
Auto-merging test/dynamo/test_higher_order_ops.py
CONFLICT (content): Merge conflict in test/dynamo/test_higher_order_ops.py
Auto-merging torch/_dynamo/exc.py
Auto-merging torch/_dynamo/variables/higher_order_ops.py
CONFLICT (content): Merge conflict in torch/_dynamo/variables/higher_order_ops.py
error: could not apply 6a310b56942... Automatically turn on dynamo in cond
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
Details for Dev Infra team Raised by workflow job

@ydwu4
Copy link
Contributor Author

ydwu4 commented Aug 28, 2023

@pytorchbot merge

@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

@ydwu4 ydwu4 mentioned this pull request Aug 28, 2023
Closed
@facebook-github-bot facebook-github-bot deleted the gh/ydwu4/14/head branch August 31, 2023 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zou3519 zou3519 zou3519 approved these changes

Assignees

No one assigned

Labels

ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: dynamo release notes: fx release notes category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ydwu4 @pytorchmergebot @zou3519