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

Fix random_mps_impl to accept non-contiguous tensors #125231

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lamroger
Copy link
Contributor

Fixes #124029

Follows the pattern of allocating contiguous memory and copying the results in https://github.com/pytorch/pytorch/pull/123049/files

Copy link

pytorch-bot bot commented Apr 30, 2024

🔗 Helpful Links

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

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

❌ 5 New Failures, 4 Unrelated Failures

As of commit 3b7efae with merge base b03fb49 (image):

NEW FAILURES - The following jobs have failed:

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

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

@pytorch-bot pytorch-bot bot added the release notes: mps Release notes category label Apr 30, 2024
@lamroger
Copy link
Contributor Author

random_mps_impl is used in other operations in Distribution. Should I add tests for those too?
cc: @kulinseth since I followed your suggestion here: #120272 (review) and think the same advice applies here

@cpuhrsch cpuhrsch requested a review from albanD April 30, 2024 19:52
@cpuhrsch cpuhrsch added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Apr 30, 2024
@albanD albanD removed their request for review April 30, 2024 20:07
Copy link
Collaborator

@kulinseth kulinseth left a comment

Choose a reason for hiding this comment

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

Looks good.

@lamroger
Copy link
Contributor Author

lamroger commented May 2, 2024

@pytorchbot merge

Copy link

pytorch-bot bot commented May 2, 2024

Pull workflow has not been scheduled for the PR yet. It could be because author doesn't have permissions to run those or skip-checks keywords were added to PR/commits, aborting merge. Please get/give approval for the workflows and/or remove skip ci decorators before next merge attempt. If you think this is a mistake, please contact PyTorch Dev Infra.

@lamroger
Copy link
Contributor Author

lamroger commented May 2, 2024

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

@pytorchmergebot
Copy link
Collaborator

Successfully rebased roger/non-contiguous-rand-like onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout roger/non-contiguous-rand-like && git pull --rebase)

@pytorchmergebot pytorchmergebot force-pushed the roger/non-contiguous-rand-like branch from 357dda6 to a569b52 Compare May 2, 2024 06:45
@lamroger
Copy link
Contributor Author

lamroger commented May 2, 2024

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label May 2, 2024
@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

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 1 jobs have failed, first few of them are: trunk / macos-12-py3-arm64-mps / test (mps, 1, 1, macos-m1-stable)

Details for Dev Infra team Raised by workflow job

@lamroger lamroger requested a review from mruberry as a code owner May 4, 2024 22:10
@lamroger lamroger requested a review from kulinseth May 4, 2024 22:12
@lamroger
Copy link
Contributor Author

lamroger commented May 4, 2024

Hey @kulinseth , wanted to double check removing this test is okay since we can accept multiple elements referencing the same location now

@@ -2003,13 +2003,6 @@ def sample_inputs_bernoulli(self, device, dtype, requires_grad, **kwargs):
requires_grad=requires_grad)
yield SampleInput(t)

def error_inputs_bernoulli(op_info, device, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this test removed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bernuoulli uses random_mps_impl under the hood so the error wasn't valid anymore since it support non-contiguous tenors now too. So I removed it but on second thought, it might be useful to keep the test as a passing test for documentation.

I added it back to the sample_inputs_bernoulli method. Open to other ideas too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request open source release notes: mps Release notes category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rand_like on permuted parameter not working on MPS
5 participants