[MPS] Enable forward test for renorm #106666

arunppsg · 2023-08-05T13:52:21Z

Enabled forward test for renorm

cc @qqaatw

pytorch-bot · 2023-08-05T13:52:23Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

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

✅ No Failures

As of commit 2f7c603 with merge base bd34f85 ():
💚 Looks good so far! There are no failures yet. 💚

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

qqaatw · 2023-08-08T08:55:31Z

Hi, can you fix the tests, please?

arunppsg · 2023-08-08T18:10:06Z

Hey, I am not sure what is causing the test failure. Can you please guide me on how to fix it?

kulinseth · 2023-08-09T14:18:58Z

Hey, I am not sure what is causing the test failure. Can you please guide me on how to fix it?

@qqaatw , thanks for the fix.

qqaatw · 2023-08-09T14:24:51Z

@arunppsg, please fix the lint, thanks.

Here are a couple of things:

Please do not merge your PR when the newly added op is not covered by tests, doing so may cause the PR to be reverted as the implementation may be incorrect. If you're not sure which tests should be enabled or xfailed, you could ask the maintainers for guidance.
The maxnorm input is a Scalar, which is actually a class structure. We should pass the underlying value of it, which is a C++ primitive data type, into the Metal kernel instead of passing the pointer of the Scalar. Additionally, Metal doesn't support double data type, so we should convert it to float in advance.
The backward of renorm has actually been implemented for all backends, see here. The issue was with data type.

arunppsg · 2023-08-09T15:10:54Z

Thank you @qqaatw for help - sure, I will keep the pointers in mind during future contribution.

arunppsg · 2023-08-15T18:08:39Z

@pytorchbot merge

pytorchmergebot · 2023-08-15T18:54:02Z

Merge failed

Reason: Approval needed from one of the following:
caogao, bugra, mergennachin, smessmer, XilunWu, ...

Details for Dev Infra team

Raised by workflow job

Failing merge rule: Core Maintainers

albanD

Random rant about our code being too cuda specific, shouldn't block this PR!

albanD · 2023-08-17T15:33:35Z

torch/csrc/autograd/FunctionsManual.cpp

-      at::toAccumulateType(self.scalar_type(), /*is_cuda=*/self.is_cuda());
+  auto acc_type = self.is_mps()
+      ? self.scalar_type()
+      : at::toAccumulateType(self.scalar_type(), /*is_cuda=*/self.is_cuda());


I wonder if we should move toAccumulateType() to have a is_gpu (or device) flag to be able to treat the accumulation type the same way on cuda and mps.

yeah I thought about this but was not sure whether the cuda accumulation types are applicable to mps or not. So didn't change this part.

Will investigate it.

qqaatw · 2023-08-17T16:44:21Z

@pytorchbot merge

pytorchmergebot · 2023-08-17T16:46:00Z

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

Trying to address this comment: #106666 (comment) cc albanD kulinseth [ghstack-poisoned]

Trying to address this comment: #106666 (comment) Pull Request resolved: #108248 Approved by: https://github.com/kulinseth, https://github.com/albanD

test for renorm

272ee66

arunppsg requested a review from kulinseth as a code owner August 5, 2023 13:52

pytorch-bot bot added ciflow/mps Run MPS tests (subset of trunk) topic: not user facing topic category labels Aug 5, 2023

pytorchbot added the open source label Aug 5, 2023

zou3519 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Aug 7, 2023

arunppsg marked this pull request as draft August 8, 2023 17:19

Fix renorm forward & backward

6f7143b

lint fix

2f7c603

arunppsg marked this pull request as ready for review August 9, 2023 16:01

arunppsg requested review from albanD and soulitzer as code owners August 9, 2023 16:01

albanD removed their request for review August 9, 2023 18:24

kulinseth approved these changes Aug 15, 2023

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Aug 15, 2023

pytorchmergebot added the merging label Aug 15, 2023

pytorchmergebot removed the merging label Aug 15, 2023

albanD approved these changes Aug 17, 2023

View reviewed changes

pytorchmergebot added the merging label Aug 17, 2023

pytorchmergebot added the Merged label Aug 17, 2023

pytorchmergebot removed the merging label Aug 17, 2023

pytorchmergebot closed this in 4bfc55b Aug 17, 2023

qqaatw mentioned this pull request Aug 30, 2023

Generalize toAccumulateType() #108248

Closed

qqaatw added a commit that referenced this pull request Sep 3, 2023

Update on "Generalize toAccumulateType()"

1765ffd

Trying to address this comment: #106666 (comment) cc albanD kulinseth [ghstack-poisoned]

qqaatw added a commit that referenced this pull request Sep 4, 2023

Update on "Generalize toAccumulateType()"

73d7844

Trying to address this comment: #106666 (comment) cc albanD kulinseth [ghstack-poisoned]

qqaatw added a commit that referenced this pull request Sep 4, 2023

Update on "Generalize toAccumulateType()"

deb1cb7

Trying to address this comment: #106666 (comment) cc albanD kulinseth [ghstack-poisoned]

qqaatw added a commit that referenced this pull request Sep 7, 2023

Update on "Generalize toAccumulateType()"

f2c8348

Trying to address this comment: #106666 (comment) cc albanD kulinseth [ghstack-poisoned]

qqaatw added a commit that referenced this pull request Sep 7, 2023

Update on "Generalize toAccumulateType()"

24ce043

Trying to address this comment: #106666 (comment) cc albanD kulinseth [ghstack-poisoned]

qqaatw added a commit that referenced this pull request Sep 12, 2023

Update base for Update on "Generalize toAccumulateType()"

7472dfd

Trying to address this comment: #106666 (comment) cc albanD kulinseth [ghstack-poisoned]

qqaatw added a commit that referenced this pull request Sep 12, 2023

Update on "Generalize toAccumulateType()"

ce83816

Trying to address this comment: #106666 (comment) cc albanD kulinseth [ghstack-poisoned]

pytorchmergebot pushed a commit that referenced this pull request Sep 29, 2023

Update base for Update on "Generalize toAccumulateType()"

92ba7c5

Trying to address this comment: #106666 (comment) cc albanD kulinseth [ghstack-poisoned]

pytorchmergebot pushed a commit that referenced this pull request Sep 29, 2023

Update on "Generalize toAccumulateType()"

d461e4b

Trying to address this comment: #106666 (comment) cc albanD kulinseth [ghstack-poisoned]

qqaatw added a commit that referenced this pull request Oct 2, 2023

Update base for Update on "Generalize toAccumulateType()"

755fe98

Trying to address this comment: #106666 (comment) cc albanD kulinseth [ghstack-poisoned]

qqaatw added a commit that referenced this pull request Oct 2, 2023

Update on "Generalize toAccumulateType()"

b9fe25c

Trying to address this comment: #106666 (comment) cc albanD kulinseth [ghstack-poisoned]

pytorchmergebot pushed a commit that referenced this pull request Oct 2, 2023

Generalize toAccumulateType() (#108248)

a3c1e3c

Trying to address this comment: #106666 (comment) Pull Request resolved: #108248 Approved by: https://github.com/kulinseth, https://github.com/albanD

[MPS] Enable forward test for renorm #106666

[MPS] Enable forward test for renorm #106666

Uh oh!

Conversation

arunppsg commented Aug 5, 2023

Uh oh!

pytorch-bot bot commented Aug 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

✅ No Failures

Uh oh!

qqaatw commented Aug 8, 2023

Uh oh!

arunppsg commented Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kulinseth commented Aug 9, 2023

Uh oh!

qqaatw commented Aug 9, 2023

Uh oh!

arunppsg commented Aug 9, 2023

Uh oh!

arunppsg commented Aug 15, 2023

Uh oh!

pytorchmergebot commented Aug 15, 2023

Merge failed

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

albanD Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

qqaatw Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

qqaatw commented Aug 17, 2023

Uh oh!

pytorchmergebot commented Aug 17, 2023

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pytorch-bot bot commented Aug 5, 2023 •

edited

Loading

arunppsg commented Aug 8, 2023 •

edited

Loading