Skip to content
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

Make TORCH_COMPILE_DEBUG=1 work again #112917

Closed
wants to merge 3 commits into from
Closed

Make TORCH_COMPILE_DEBUG=1 work again #112917

wants to merge 3 commits into from
+3 −2

Conversation

ipiszy
Copy link
Contributor

@ipiszy ipiszy commented Nov 3, 2023 •
edited by pytorch-bot bot

ATT. After the fix, self.node is Optional[ir.Buffer] in FusedSchedulerNode and ForeachKernelSchedulerNode, but ir.Buffer in BaseSchedulerNode. Using ir.Buffer for BaseSchedulerNode.node avoids all mypy complaints about Optionals.

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @peterbell10 @yf225 @chenyang78 @kadeng @muchulee8 @aakhundov @ColinPeppler

@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Nov 3, 2023 •
edited

🔗 Helpful Links

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

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

✅ You can merge normally! (2 Unrelated Failures)

As of commit 2e8bff9 with merge base 132cb57 (image):

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

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

@ipiszy ipiszy linked an issue Nov 3, 2023 that may be closed by this pull request
Inductor compile failed with enabling debug log #112690
Closed
@ipiszy
Copy link
Contributor Author

ipiszy commented Nov 3, 2023

topic: not user facing

@davidberard98 davidberard98 added the topic: not user facing topic category label Nov 3, 2023
davidberard98
davidberard98 approved these changes Nov 3, 2023
View reviewed changes
Copy link
Contributor

@davidberard98 davidberard98 left a comment

Choose a reason for hiding this comment

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

thanks Ying!

@facebook-github-bot
Copy link
Contributor

@ipiszy has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

int3
int3 approved these changes Nov 3, 2023
View reviewed changes
Copy link
Contributor

@int3 int3 left a comment

Choose a reason for hiding this comment

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

Surprised there are no new type errors. Thanks for the fix!

test/inductor/test_torchinductor.py
@@ -3111,6 +3111,7 @@ def forward(self, l_input_: torch.Tensor):
o2 = torch.compile(mod)(inp)
self.assertEqual(o1, o2)

@patch.object(config.trace, "enabled", True)
Copy link
Contributor

Choose a reason for hiding this comment

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

why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To add a test with "TORCH_COMPILE_DEBUG=1". It seems that we don't have any test coverage for "TORCH_COMPILE_DEBUG=1" for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably just make a separate sanity test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Chillee Where do you suggest to put this sanity test?

@ipiszy
Copy link
Contributor Author

ipiszy commented Nov 3, 2023

Surprised there are no new type errors. Thanks for the fix!

Yeah, I'm surprised, too...

@ipiszy
Copy link
Contributor Author

ipiszy commented Nov 3, 2023

Surprised there are no new type errors. Thanks for the fix!

There is actually a mypy error, for some reason my local run doesn't show this error:

Incompatible types in assignment (expression has type "Optional[Buffer]",
2023-11-03T22:55:19.6640685Z     base class "BaseSchedulerNode" defined the type as "Buffer")

So need to fix it properly..

@facebook-github-bot
Copy link
Contributor

@ipiszy has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@aakhundov
Copy link
Contributor

I think, self.node: ir.Buffer = None --> self.node: Optional[ir.Buffer] = None should fix the mypy error?

@facebook-github-bot
Copy link
Contributor

@ipiszy has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@ipiszy
Copy link
Contributor Author

ipiszy commented Nov 7, 2023

I think, self.node: ir.Buffer = None --> self.node: Optional[ir.Buffer] = None should fix the mypy error?

It will introduce other mypy errors lol. I'm just going to add "type: ignore" here.

@facebook-github-bot
Copy link
Contributor

@ipiszy has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@ipiszy
Copy link
Contributor Author

ipiszy commented Nov 7, 2023

@pytorchbot merge

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

@leslie-fang-intel leslie-fang-intel mentioned this pull request Nov 14, 2023
Closed
Skylion007 pushed a commit to Skylion007/pytorch that referenced this pull request Nov 14, 2023
@ipiszy @Skylion007
Make TORCH_COMPILE_DEBUG=1 work again (pytorch#112917)
719f7d4 
ATT. After the fix, self.node is `Optional[ir.Buffer]` in `FusedSchedulerNode` and `ForeachKernelSchedulerNode`, but `ir.Buffer` in `BaseSchedulerNode`. Using `ir.Buffer` for `BaseSchedulerNode.node` avoids all mypy complaints about Optionals.

Pull Request resolved: pytorch#112917
Approved by: https://github.com/davidberard98, https://github.com/int3, https://github.com/leslie-fang-intel, https://github.com/aakhundov
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Chillee Chillee Chillee left review comments

@int3 int3 int3 approved these changes

@davidberard98 davidberard98 davidberard98 approved these changes

@aakhundov aakhundov aakhundov approved these changes

@leslie-fang-intel leslie-fang-intel leslie-fang-intel approved these changes

Assignees
No one assigned
Labels
ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: inductor topic: not user facing topic category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inductor compile failed with enabling debug log
8 participants
@ipiszy @facebook-github-bot @aakhundov @pytorchmergebot @int3 @davidberard98 @Chillee @leslie-fang-intel