Skip to content

Conversation

@malfet
Copy link
Contributor

@malfet malfet commented Dec 20, 2022

Stack from ghstack (oldest at bottom):

Use Prims to implement group_norm, group_norm_backward and mean_var

Use torch._ops.ops instead of torch.ops in numerous subpackages in
order to be able to make them importable from torch/backend/mps/__init__.py as this alias is defined in

from torch._ops import ops

is executed last during init process.

Add __all__ to torch/backends/mps/__init__.py as well as alias all imports as private

Add TestNNMPS.test_group_norm_backward that validates no NaNs are generated during the backward pass

Fixes #88331

Use Prims to implement group_norm, group_norm_backward and mean_var

Use `torch._ops.ops` instead of `torch.ops` in numerous subpackages in
order to be able to register python implementation in mps backend

TODO: Fix MPS var implementation and add it to prims

Fixes #88331

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Dec 20, 2022

🔗 Helpful Links

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

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

✅ No Failures

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

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

@malfet
Copy link
Contributor Author

malfet commented Dec 20, 2022

I wonder if it'll be better done using @torch.library.impl decorator in the respective files

@malfet malfet changed the title [MPS] Add group_norm[fwd+backward] and mean_var [MPS] Add group_norm[fwd+backward] and mean_var Dec 20, 2022
Use Prims to implement group_norm, group_norm_backward and mean_var

Use `torch._ops.ops` instead of `torch.ops` in numerous subpackages in
order to be able to register python implementation in mps backend

TODO: Fix MPS var implementation and add it to prims

Fixes #88331

[ghstack-poisoned]
malfet added a commit that referenced this pull request Dec 20, 2022
Use Prims to implement group_norm, group_norm_backward and mean_var

Use `torch._ops.ops` instead of `torch.ops` in numerous subpackages in
order to be able to register python implementation in mps backend

TODO: Fix MPS var implementation and add it to prims

Fixes #88331

ghstack-source-id: 1437e0e
Pull Request resolved: #91190
@malfet malfet requested review from kulinseth and ngimel December 20, 2022 23:06
Use Prims to implement group_norm, group_norm_backward and mean_var

Use `torch._ops.ops` instead of `torch.ops` in numerous subpackages in
order to be able to register python implementation in mps backend

TODO: Fix MPS var implementation and add it to prims

Fixes #88331

[ghstack-poisoned]
@malfet malfet added ciflow/mps Run MPS tests (subset of trunk) topic: improvements topic category release notes: mps Release notes category labels Dec 21, 2022
Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

Can you explain in more details what is the difference between torch.ops and torch._ops.ops ?
As far as I can tell they are exactly the same thing.

The new registration looks good.

@malfet
Copy link
Contributor Author

malfet commented Dec 21, 2022

@pytorchbot merge -f "MPS tests are passing"

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

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

@kit1980
Copy link
Contributor

kit1980 commented Dec 21, 2022

@pytorchbot revert -m "Broke test_correct_module_names because of underscore _ops" -c weird

@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

@malfet your PR has been successfully reverted.

pytorchmergebot added a commit that referenced this pull request Dec 21, 2022
This reverts commit 371716e.

Reverted #91190 on behalf of https://github.com/kit1980 due to Broke test_correct_module_names because of underscore _ops
@malfet malfet reopened this Dec 22, 2022
Use Prims to implement group_norm, group_norm_backward and mean_var

Use `torch._ops.ops` instead of `torch.ops` in numerous subpackages in
order to be able to make them importable from `torch/backend/mps/__init__.py` as this alias is defined in 
https://github.com/pytorch/pytorch/blob/15af4b1ceea3b689354ad664675b930486804c8e/torch/__init__.py#L1095
is executed last during init process.

Depends on #91203

Fixes #88331

[ghstack-poisoned]
@malfet malfet added the ciflow/trunk Trigger trunk jobs on your pull request label Dec 22, 2022
@malfet malfet changed the title [MPS] Add group_norm[fwd+backward] and mean_var [MPS] Add group_norm[fwd+backward] and mean_var (take 2) Dec 22, 2022
Use Prims to implement group_norm, group_norm_backward and mean_var

Use `torch._ops.ops` instead of `torch.ops` in numerous subpackages in
order to be able to make them importable from `torch/backend/mps/__init__.py` as this alias is defined in 
https://github.com/pytorch/pytorch/blob/15af4b1ceea3b689354ad664675b930486804c8e/torch/__init__.py#L1095
is executed last during init process.

Depends on #91203

Fixes #88331

[ghstack-poisoned]
Use Prims to implement group_norm, group_norm_backward and mean_var

Use `torch._ops.ops` instead of `torch.ops` in numerous subpackages in
order to be able to make them importable from `torch/backend/mps/__init__.py` as this alias is defined in 
https://github.com/pytorch/pytorch/blob/15af4b1ceea3b689354ad664675b930486804c8e/torch/__init__.py#L1095
is executed last during init process.

Add `__all__` to `torch/backends/mps/__init__.py` as well as alias all imports as private

Add `TestNNMPS.test_group_norm_backward` that validates no NaNs are generated during the backward pass

Fixes #88331

[ghstack-poisoned]
malfet added a commit that referenced this pull request Dec 22, 2022
Use Prims to implement group_norm, group_norm_backward and mean_var

Use `torch._ops.ops` instead of `torch.ops` in numerous subpackages in
order to be able to register python implementation in mps backend

TODO: Fix MPS var implementation and add it to prims

Fixes #88331

ghstack-source-id: 43325e0
Pull Request resolved: #91190
@malfet
Copy link
Contributor Author

malfet commented Dec 22, 2022

@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

@facebook-github-bot facebook-github-bot deleted the gh/malfet/51/head branch June 8, 2023 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/inductor ciflow/mps Run MPS tests (subset of trunk) ciflow/trunk Trigger trunk jobs on your pull request Merged release notes: fx release notes category release notes: mps Release notes category Reverted topic: improvements topic category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants