🔗 Helpful Links

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

• 📄 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 7be6762 with merge base 2247aa6 ():
💚 Looks good so far! There are no failures yet. 💚

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

Attention! native_functions.yaml was changed

If you are adding a new function or defaulted argument to native_functions.yaml, you cannot use it from pre-existing Python frontend code until our FC window passes (two weeks). Split your PR into two PRs, one which adds the new C++ functionality, and one that makes use of it from Python, and land them two weeks apart. See https://github.com/pytorch/pytorch/wiki/PyTorch's-Python-Frontend-Backward-and-Forward-Compatibility-Policy#forwards-compatibility-fc for more info.

Caused by:

• aten/src/ATen/native/native_functions.yaml

I don't think that test is needed. indices() and values() do not come from a custom implementation and I would basically be comparing two same things to each other, just being dispatched from different devices.

I don't think that test is needed. indices() and values() do not come from a custom implementation

These tests are needed for sanity. Note that indices/values belong to the public API, not _indices/_values.

and I would basically be comparing two same things to each other, ...

Isn't that true for most tests? :)

..., just being dispatched from different devices.

This is also the reason why these tests are needed as this PR adds a feature to the dispatching system.

It's not true for most of the tests. Tests which compare outputs between device and cpu, in my opinion, should be added when you have a different implementation for it. i.e. see implementation for coalesce_sparse_mps. coalesce_sparse_mps and coalesce_sparse_cpu have different underlying implementations/functions they call, thus the need to check for the outputs to match. Whenever you dispatch the function to the same underlying function which is the case for indices and values, test comparing cpu output and mps output isn't needed.

Also one of the reasons tests get added is to make sure that in the future people don't break things, here you can't physically break this implementation since you will be breaking both mps/cpu/cuda implementation if you change the underlying function.

@malfet What do you think?

Also one of the reasons tests get added is to make sure that in the future people don't break things, here you can't physically break this implementation since you will be breaking both mps/cpu/cuda implementation if you change the underlying function.

There exists a number of ways to break a code that relies on the feature that this PR introduces without pytorch CI discovering the breakage. A trivial example is a future PR that removes the dispatch keys introduced here. Or a PR that provides a MPS-specific-but-buggy implementation for indices/values. Etc. While these examples may be artificial (or are they? This is considering that people will use/already use AI to generate PRs) in the sense that the review process should prevent landing such PRs. A failing CI would save everyone's time by making such future PRs unacceptable.

@pytorchbot merge -f "This seems fine"

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team