Adds reference vs. noncontiguous OpInfo test#67434
Closed
Conversation
CI Flow Status⚛️ CI FlowRuleset - Version:
You can add a comment to the PR and tag @pytorchbot with the following commands: # ciflow rerun, "ciflow/default" will always be added automatically
@pytorchbot ciflow rerun
# ciflow rerun with additional labels "-l <ciflow/label_name>", which is equivalent to adding these labels manually and trigger the rerun
@pytorchbot ciflow rerun -l ciflow/scheduled -l ciflow/slowFor more information, please take a look at the CI Flow Wiki. |
Contributor
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 52b5109 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
Collaborator
Author
|
This will still require a follow-up issue to evaluate whether noncontiguous sample inputs should be used in more contexts (like when testing autograd) and if they can be removed from existing sample input functions. |
Contributor
|
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
ngimel
reviewed
Oct 29, 2021
| requires_grad=requires_grad), | ||
| args=(make_tensor(second_shape, device, dtype, | ||
| requires_grad=requires_grad),))) | ||
| SampleInput(make_arg(first_shape).requires_grad_(requires_grad), |
Collaborator
There was a problem hiding this comment.
nit: make_arg(first_shape, requires_grad=requires_grad), partial accepts that
Collaborator
Author
There was a problem hiding this comment.
Nice catch -- fixed.
ngimel
reviewed
Oct 29, 2021
| for tensor, idx, source, a in product([t, t_nonctg], [idx, idx_nonctg], [s, s_nonctg], [-1, 0, 2])) | ||
|
|
||
| samples = [SampleInput(t.detach().clone().requires_grad_(requires_grad), | ||
| args=(1, idx.detach().clone(), s.detach().clone()))] |
Collaborator
There was a problem hiding this comment.
did you lose s's requires_grad here? Would autograd tests normally check all inputs that require grad?
Collaborator
Author
There was a problem hiding this comment.
Great catch -- yes I did on the s (source) tensor
ngimel
approved these changes
Oct 29, 2021
Contributor
|
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Contributor
|
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Contributor
|
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
added 2 commits
October 29, 2021 14:22
…h into noncontiguous_tests
Contributor
|
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Contributor
laurentdupin
pushed a commit
to laurentdupin/pytorch
that referenced
this pull request
Apr 25, 2026
Summary: Fixes pytorch#63341. This PR adds a new test, `test_noncontigous_samples`, that runs ops forward and backward and compares their outputs and grads between "normal" contiguous SampleInputs and noncontiguous SampleInputs. This test should preclude the need for noncontiguous SampleInputs going forward. The test was added by generalizing the `.numpy()` transform on SampleInputs to support a new `.noncontiguous()` transform and copying forward/backward patterns from other tests in test_ops.py. It also discovered that many SampleInputs were incorrectly reusing tensors, so those have been revised. SampleInputs creating noncontiguous tensors for testing have also been altered to no longer do so. In addition, this test discovered the following high priority silent correctness issues: - pytorch#67432 - pytorch#67517 - pytorch#67513 - pytorch#67512 - pytorch#67470 It also identified the following issues: - pytorch#67539 The pow OpInfo also incorrectly specified that pow supported the bool datatype, and this has been fixed. Its SampleInputs were written in a way that made requests for boolean SampleInputs return type promoting inputs that never actually tried to compute pow in bool. This PR suggests we should add the following guidance for writing SampleInputs: - ensure that all SampleInputs are independent of each other (don't reuse tensors) - ensure that all SampleInput tensors have no grad or backward functions (no autograd history) -- they should be leaves - prefer keeping sample inputs simple where possible, a good set of handwritten samples that test interesting cases may be better than an exhaustive but hard to read and maintain programmatic enumeration - keep code readable by using functools.partial and writing simple inline helpers; break up large statements into a more readable series of smaller statements; especially don't write complicated generator expressions with a `for` at the end! fyi kshitij12345 krshrimali pmeier anjali411 saketh-are zou3519 dagitses Pull Request resolved: pytorch#67434 Reviewed By: ngimel Differential Revision: D32014557 Pulled By: mruberry fbshipit-source-id: b17e19adc1d41e24441f0765af13d381fef5e3c1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #63341.
This PR adds a new test,
test_noncontigous_samples, that runs ops forward and backward and compares their outputs and grads between "normal" contiguous SampleInputs and noncontiguous SampleInputs. This test should preclude the need for noncontiguous SampleInputs going forward.The test was added by generalizing the
.numpy()transform on SampleInputs to support a new.noncontiguous()transform and copying forward/backward patterns from other tests in test_ops.py. It also discovered that many SampleInputs were incorrectly reusing tensors, so those have been revised. SampleInputs creating noncontiguous tensors for testing have also been altered to no longer do so.In addition, this test discovered the following high priority silent correctness issues:
It also identified the following issues:
The pow OpInfo also incorrectly specified that pow supported the bool datatype, and this has been fixed. Its SampleInputs were written in a way that made requests for boolean SampleInputs return type promoting inputs that never actually tried to compute pow in bool.
This PR suggests we should add the following guidance for writing SampleInputs:
forat the end!fyi @kshitij12345 @krshrimali @pmeier @anjali411 @saketh-are @zou3519 @dagitses