KEMBAR78
Assert that output could only be the last node of the FX graph by oulgen · Pull Request #114973 · pytorch/pytorch · GitHub
Skip to content

Assert that output could only be the last node of the FX graph #114973

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 1 commit into from
Closed

Assert that output could only be the last node of the FX graph #114973

wants to merge 1 commit into from

Conversation

@oulgen
Copy link
Contributor

@oulgen oulgen commented Dec 1, 2023 •
edited
Loading

@pytorch-bot pytorch-bot bot added the release notes: fx release notes category label Dec 1, 2023
@pytorch-bot
Copy link

pytorch-bot bot commented Dec 1, 2023 •
edited
Loading

🔗 Helpful Links

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

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 2344170 with merge base 6f32eb7 (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.

oulgen added a commit that referenced this pull request Dec 1, 2023
@oulgen
Assert that output could only be the last node of the FX graph
1455a3d 
ghstack-source-id: 70fc4eb
Pull Request resolved: #114973
@oulgen oulgen added the ciflow/trunk Trigger trunk jobs on your pull request label Dec 1, 2023
@oulgen oulgen marked this pull request as ready for review December 1, 2023 20:49
@oulgen oulgen added the topic: not user facing topic category label Dec 1, 2023
Chillee
Chillee approved these changes Dec 1, 2023
View reviewed changes
Copy link
Collaborator

@Chillee Chillee left a comment

Choose a reason for hiding this comment

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

LGTM!

@suo
Copy link
Member

suo commented Dec 1, 2023

Apparently we don't call lint() anywhere in the normal flow of compilation, so we'll have to add that somewhere reasonable (post-aotautograd?) if we want this to actually be an invariant we rely on.

@oulgen
Copy link
Contributor Author

oulgen commented Dec 1, 2023 •
edited
Loading

@suo let me do that as follow up on this PR. If I were to add lint compile_fx in inductor, @Chillee you mentioned the perf problems. So what would be a good way to add this?

One option is to add it under an inductor config flag and only enable this flag by default on tests (is there a way to do this?)

Any alternatives?

@Chillee
Copy link
Collaborator

Chillee commented Dec 1, 2023

We currently call it here most often I think: https://github.com/pytorch/pytorch/blob/main/torch/_functorch/partitioners.py#L104

@oulgen
Copy link
Contributor Author

oulgen commented Dec 1, 2023

We currently call it here most often I think: https://github.com/pytorch/pytorch/blob/main/torch/_functorch/partitioners.py#L104

Are you suggesting that is enough?

@Chillee
Copy link
Collaborator

Chillee commented Dec 1, 2023

Yeah I think so.

@oulgen
Copy link
Contributor Author

oulgen commented Dec 1, 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

@atalman
Copy link
Contributor

atalman commented Dec 4, 2023

@pytorchbot revert -m="Diff broke internal tests" -c="ghfirst"

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

@pytorchmergebot
Copy link
Collaborator

@oulgen your PR has been successfully reverted.

pytorchmergebot added a commit that referenced this pull request Dec 4, 2023
@pytorchmergebot
Revert "Assert that output could only be the last node of the FX graph (
8bb3cd1 
#114973)"

This reverts commit a85df9e.

Reverted #114973 on behalf of https://github.com/atalman due to Diff broke internal tests ([comment](#114973 (comment)))
@oulgen
Copy link
Contributor Author

oulgen commented Dec 6, 2023

Merged again as #115179

@oulgen oulgen closed this Dec 6, 2023
@facebook-github-bot facebook-github-bot deleted the gh/oulgen/38/head branch December 10, 2023 15:27
dmenig pushed a commit to dmenig/pytorch that referenced this pull request Dec 21, 2023
@oulgen @dmenig
Assert that output could only be the last node of the FX graph (pytor…
4ef22d0 
…ch#114973)

Pull Request resolved: pytorch#114973
Approved by: https://github.com/Chillee
dmenig pushed a commit to dmenig/pytorch that referenced this pull request Dec 21, 2023
@pytorchmergebot @dmenig
Revert "Assert that output could only be the last node of the FX graph (
db502ba 
pytorch#114973)"

This reverts commit a85df9e.

Reverted pytorch#114973 on behalf of https://github.com/atalman due to Diff broke internal tests ([comment](pytorch#114973 (comment)))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Chillee Chillee Chillee approved these changes

@jansel jansel Awaiting requested review from jansel

@suo suo Awaiting requested review from suo

@voznesenskym voznesenskym Awaiting requested review from voznesenskym

Assignees

No one assigned

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged release notes: fx release notes category Reverted topic: not user facing topic category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@oulgen @suo @Chillee @pytorchmergebot @atalman