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

Add new aten::device variant to TorchScript #97023

Closed
wants to merge 1 commit into from
Closed

Add new aten::device variant to TorchScript #97023

wants to merge 1 commit into from

Conversation

thiagocrepaldi
Copy link
Collaborator

@thiagocrepaldi thiagocrepaldi commented Mar 17, 2023 •
edited by pytorch-bot bot

@pytorch-bot
Copy link

pytorch-bot bot commented Mar 17, 2023 •
edited

🔗 Helpful Links

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

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

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

✅ No Failures

As of commit caea625:
💚 Looks good so far! There are no failures yet. 💚

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

@pytorch-bot pytorch-bot bot added the release notes: mobile release notes category label Mar 17, 2023
@thiagocrepaldi thiagocrepaldi mentioned this pull request Mar 17, 2023
Closed
@davidberard98
Copy link
Contributor

Feel free to add me as a reviewer when you need the PR to be reviewed.

@thiagocrepaldi
Copy link
Collaborator Author

Feel free to add me as a reviewer when you need the PR to be reviewed.

Thank you. The current draft tries to add a variant to aten::device, but apparently something is missing because I got the exact same error. Any clues?

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 17, 2023 •
edited

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: thiagocrepaldi / name: Thiago Crepaldi (3c97bc1)

@thiagocrepaldi thiagocrepaldi self-assigned this Mar 17, 2023
@thiagocrepaldi thiagocrepaldi added the module: onnx Related to torch.onnx label Mar 17, 2023
@davidberard98
Copy link
Contributor

^^ not sure if that's the issue. we can wait for CI to see if you recent changes fixed the previous errors in CI.

@davidberard98 davidberard98 self-requested a review March 17, 2023 21:17
@pytorch-bot
Copy link

pytorch-bot bot commented Mar 20, 2023

No ciflow labels are configured for this repo.
For information on how to enable CIFlow bot see this wiki

@kit1980
Copy link
Member

kit1980 commented Mar 28, 2023

To resolve the CLA issue, make sure to add the email address of the commit to your GitHub profile
(to see the actual email address, add .patch at the end of this PR page's URL).

@thiagocrepaldi
Copy link
Collaborator Author

To resolve the CLA issue, make sure to add the email address of the commit to your GitHub profile (to see the actual email address, add .patch at the end of this PR page's URL).

image
does it have to be the "main" email?

https://patch-diff.githubusercontent.com/raw/pytorch/pytorch/pull/97023.patch shows thiago.crepaldi@microsoft.com which is already on my github profile, as seen above

jgong5
jgong5 approved these changes Mar 30, 2023
View reviewed changes
Copy link
Collaborator

@jgong5 jgong5 left a comment

Choose a reason for hiding this comment

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

would you please add unit test?

@thiagocrepaldi
Copy link
Collaborator Author

**pull / linux-focal-py3-clang7-mobile-build / build (pull_request) ** Failing after 8m

Sure, I can add unit test when I find out what is going on with the pull / linux-focal-py3-clang7-mobile-build / build (pull_request) Failing after 8m and pull / linux-focal-py3-clang7-mobile-custom-build-static / build (pull_request)

It complains about aten::device being already registered. Any ideas? Funny this only happens for mobile pipelines.

@davidberard98 do you have any idea of what might be happening?

@thiagocrepaldi thiagocrepaldi marked this pull request as ready for review March 31, 2023 19:37
@thiagocrepaldi

This comment was marked as outdated.

Sign in to view
@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Apr 5, 2023
@pytorchmergebot

This comment was marked as outdated.

Sign in to view
@pytorch pytorch deleted a comment from pytorch-bot bot Apr 5, 2023
@pytorch pytorch deleted a comment from pytorch-bot bot Apr 5, 2023
@thiagocrepaldi
Copy link
Collaborator Author

@davidberard98 gentle ping

davidberard98
davidberard98 approved these changes Apr 5, 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.

I think @BowenBao has outstanding nits, otherwise lgtm!

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 5, 2023 •
edited

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: thiagocrepaldi / name: Thiago Crepaldi (caea625)

Add aten::device with with_index overload to TorchScript
caea625 
The new operator is replaced by constant through peephole jit pass
@davidberard98
Copy link
Contributor

@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

ZainRizvi pushed a commit that referenced this pull request Apr 19, 2023
@ZainRizvi
Add new aten::device variant to TorchScript (#97023)
e34e812 
Fixes #96627

Pull Request resolved: #97023
Approved by: https://github.com/jgong5, https://github.com/BowenBao, https://github.com/davidberard98
@janeyx99 janeyx99 mentioned this pull request Jul 24, 2023
Closed
@thiagocrepaldi thiagocrepaldi deleted the thiagofc/fix-pytorch-96627 branch October 31, 2023 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidberard98 davidberard98 davidberard98 approved these changes

@jgong5 jgong5 jgong5 approved these changes

@BowenBao BowenBao BowenBao approved these changes

@abock abock Awaiting requested review from abock

Assignees

@thiagocrepaldi thiagocrepaldi

Labels
ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: dynamo module: onnx Related to torch.onnx open source release notes: mobile release notes category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pytorch 2 Compile Doesn't with T5 Models + OnnxRT + Cuda
7 participants
@thiagocrepaldi @davidberard98 @kit1980 @pytorchmergebot @jgong5 @BowenBao @pytorchbot