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.

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

[Dynamo][10/N] Remove TorchVariable and is_allowed #116312

Closed
wants to merge 23 commits into from

Conversation

yanboliang
Copy link
Contributor

@yanboliang yanboliang commented Dec 22, 2023

After this refactor:

  • TorchVariable definition and all references are removed.
  • All is_allowed references except one are removed.
    • The only left one is in torch/_dynamo/decorators:_disallow_in_graph_helper. It was called when users put disallow_in_graph decorator on a function. Since we use the lists in trace_rules to decide the function's trace rule, so the decorator would only be used as customer function rather than torch functions. I'll defer this to a separate decorator refactor PR.

cc @mrshenli @pritamdamania87 @zhaojuanmao @satgera @rohan-varma @gqchen @aazzolini @osalpekar @jiayisuse @H-Huang @kwen2501 @awgu @penguinwu @fegin @XilunWu @wanchaol @fduwjj @wz337 @tianyu-l @wconstab @yf225 @voznesenskym @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @aakhundov @kadeng

Copy link

pytorch-bot bot commented Dec 22, 2023

🔗 Helpful Links

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

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

✅ No Failures

As of commit 6a95351 with merge base ad3c0b2 (image):
💚 Looks good so far! There are no failures yet. 💚

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

@github-actions github-actions bot added oncall: distributed Add this issue/PR to distributed oncall triage queue module: dynamo ciflow/inductor labels Dec 22, 2023
@yanboliang yanboliang marked this pull request as draft December 22, 2023 02:01
@yanboliang yanboliang added the topic: not user facing topic category label Dec 22, 2023
@yanboliang yanboliang changed the title CI Testing [Dynamo][10/N] Remove TorchVariable and is_allowed Dec 23, 2023
@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

Reverting PR 116312 failed

Reason: Command git -C /home/runner/work/pytorch/pytorch revert --no-edit 015bd0e0a189f929e469c6bc75fe1541c18a014d returned non-zero exit code 1

Auto-merging torch/_dynamo/trace_rules.py
CONFLICT (content): Merge conflict in torch/_dynamo/trace_rules.py
Auto-merging torch/_dynamo/variables/builder.py
CONFLICT (content): Merge conflict in torch/_dynamo/variables/builder.py
Auto-merging torch/_dynamo/variables/builtin.py
Auto-merging torch/_dynamo/variables/torch.py
error: could not revert 015bd0e0a18... [Dynamo][10/N] Remove TorchVariable and is_allowed (#116312)
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git revert --continue".
hint: You can instead skip this commit with "git revert --skip".
hint: To abort and get back to the state before "git revert",
hint: run "git revert --abort".
Details for Dev Infra team Raised by workflow job

pytorchmergebot added a commit that referenced this pull request Dec 26, 2023
…actor (#116365)"

This reverts commit 951da38.

Reverted #116365 on behalf of https://github.com/kit1980 due to Need to revert this because of #116312 ([comment](#116365 (comment)))
@kit1980
Copy link
Member

kit1980 commented Dec 26, 2023

@pytorchbot revert -m "breaking internal builds" -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

@yanboliang your PR has been successfully reverted.

pytorchmergebot added a commit that referenced this pull request Dec 26, 2023
This reverts commit 015bd0e.

Reverted #116312 on behalf of https://github.com/kit1980 due to breaking internal builds ([comment](#116312 (comment)))
@malfet
Copy link
Contributor

malfet commented Dec 27, 2023

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Can't merge closed PR #116312

@kit1980
Copy link
Member

kit1980 commented Dec 27, 2023

@pytorchbot merge

@malfet Why do you try to re-merge without changes?

@malfet
Copy link
Contributor

malfet commented Dec 27, 2023

@pytorchbot merge

@malfet Why do you try to re-merge without changes?

@kit1980 typed in a wrong window, but decided not to cancel the merge as PR is already closed, so it should have been a no-op. (as you can see from the comment)

@yanboliang yanboliang restored the allowed2 branch December 27, 2023 04:33
@yanboliang yanboliang reopened this Dec 27, 2023
@huydhn
Copy link
Contributor

huydhn commented Dec 27, 2023

Looking at the unit test failures on the abandoned diff D52402227 /executorch/backends/example:test_example_delegate, I think that failed test doesn't even run on ExecuTorch OSS CI atm https://github.com/pytorch/executorch/blob/main/pytest.ini#L12-L40, so it explains why OSS CI missed it. @guangy10 Do you know any blocker adding the test to ExecuTorch OSS CI?

@yanboliang
Copy link
Contributor Author

@huydhn Yea, I have incorporated the fix into this PR, but it’s better executorch side can add this unit test to OSS.

@yanboliang
Copy link
Contributor Author

@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

@yanboliang yanboliang deleted the allowed2 branch December 27, 2023 18:47
facebook-github-bot pushed a commit to pytorch/benchmark that referenced this pull request Jan 3, 2024
Summary:
After this refactor:
* ```TorchVariable``` definition and all references are removed.
* All ```is_allowed``` references except one are removed.
  - The only left one is in ```torch/_dynamo/decorators:_disallow_in_graph_helper```. It was called when users put ```disallow_in_graph``` decorator on a function. Since we use the lists in ```trace_rules``` to decide the function's trace rule, so the decorator would only be used as customer function rather than torch functions. I'll defer this to a separate decorator refactor PR.

X-link: pytorch/pytorch#116312
Approved by: https://github.com/jansel

Reviewed By: izaitsevfb

Differential Revision: D52491036

Pulled By: yanboliang

fbshipit-source-id: 2c6e4775fdee15870d07e2095bc87f655c13a685
yanboliang added a commit to yanboliang/pytorch that referenced this pull request Jan 11, 2024
yanboliang added a commit to yanboliang/pytorch that referenced this pull request Jan 12, 2024
After this refactor:
* ```TorchVariable``` definition and all references are removed.
* All ```is_allowed``` references except one are removed.
  - The only left one is in ```torch/_dynamo/decorators:_disallow_in_graph_helper```. It was called when users put ```disallow_in_graph``` decorator on a function. Since we use the lists in ```trace_rules``` to decide the function's trace rule, so the decorator would only be used as customer function rather than torch functions. I'll defer this to a separate decorator refactor PR.

Pull Request resolved: pytorch#116312
Approved by: https://github.com/jansel
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: dynamo oncall: distributed Add this issue/PR to distributed oncall triage queue Reverted topic: not user facing topic category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants