Skip to content

Re-add Tensor.T #21175

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.

Sign up for GitHub

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

Jump to bottom
Closed
umanwizard wants to merge 9 commits into from
Closed

Re-add Tensor.T #21175

umanwizard wants to merge 9 commits into from

Conversation

@umanwizard
Copy link
Contributor

@umanwizard umanwizard commented May 30, 2019 •
edited
Loading

Something flaky is going on with test_inplace_view_saved_output on Windows.

With my PR #20598 applied, the test fails, even though there is no obvious reason it should be related, so the PR was reverted.

Based on commenting out various parts of my change and re-building, I think the problem is with the name -- renaming everything from T to asdf seems to make the test stop failing. I can't be sure that this is actually the case though, since I could just be seeing patterns in non-deterministic build output...

I spoke with @colesbury offline and we agreed that it is okay to just disable this test on Windows for now and not block landing the main change. He will look into why it is failing.

Test Plan: I will wait to make sure the Windows CI suite passes before landing this.

@umanwizard umanwizard requested review from colesbury and ezyang May 30, 2019 22:24
@pytorchbot pytorchbot added module: autograd Related to torch.autograd, and the autograd engine in general module: docs Related to our documentation, both in docs/ and docblocks module: internals Related to internal abstractions in c10 and ATen module: operators module: pybind Related to our Python bindings / interactions with other Python libraries labels May 30, 2019
@umanwizard umanwizard mentioned this pull request May 30, 2019
Open
facebook-github-bot
facebook-github-bot reviewed May 30, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@umanwizard has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ezyang
Copy link
Contributor

ezyang commented May 31, 2019

Hypothesis: Windows is sorting capitalized names differently than uncapitalized names. If this is true, any test_A (where A is a capital letter) will trigger the problem, while test_a will not.

@umanwizard
Copy link
Contributor Author

umanwizard commented May 31, 2019

@pytorchbot retest this please

@umanwizard
Copy link
Contributor Author

umanwizard commented May 31, 2019

@ezyang I don't think it is related to the order of tests executing, since running test_inplace_view_saved_output in isolation exhibits the same behavior.

On the other hand, I wouldn't be surprised if capitalization is involved somehow, e.g. with symbols in the binary being sorted in a different order and invoking some kind of non-determinism.

@umanwizard
Copy link
Contributor Author

umanwizard commented May 31, 2019

@pytorchbot rebase this please

@umanwizard
Copy link
Contributor Author

umanwizard commented May 31, 2019

@pytorchbot retest this please

facebook-github-bot
facebook-github-bot reviewed May 31, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@umanwizard has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@umanwizard
Copy link
Contributor Author

umanwizard commented Jun 3, 2019

@pytorchbot rebase this please

@ezyang ezyang removed their request for review June 4, 2019 14:48
gchanan
gchanan requested changes Jun 4, 2019
View reviewed changes
Copy link
Contributor

@gchanan gchanan left a comment

Choose a reason for hiding this comment

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

you have onnx changes, otherwise looks good.

Brennan Vincent added 2 commits June 4, 2019 14:48
facebook-github-bot
facebook-github-bot reviewed Jun 4, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@umanwizard has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot
facebook-github-bot reviewed Jun 4, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@umanwizard has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jun 5, 2019

@umanwizard merged this pull request in e268fc9.

zdevito pushed a commit to zdevito/ATen that referenced this pull request Jun 5, 2019
@facebook-github-bot
Re-add Tensor.T (#21175)
32bc465 
Summary:
Something flaky is going on with `test_inplace_view_saved_output` on Windows.

With my PR #20598 applied, the test fails, even though there is no obvious reason it should be related, so the PR was reverted.

Based on commenting out various parts of my change and re-building, I think the problem is with the name -- renaming everything from `T` to `asdf` seems to make the test stop failing. I can't be sure that this is actually the case though, since I could just be seeing patterns in non-deterministic build output...

I spoke with colesbury offline and we agreed that it is okay to just disable this test on Windows for now and not block landing the main change. He will look into why it is failing.

**Test Plan:** I will wait to make sure the Windows CI suite passes before landing this.
Pull Request resolved: pytorch/pytorch#21175

Differential Revision: D15566970

Pulled By: umanwizard

fbshipit-source-id: edf223375d41faaab0a3a14dca50841f08030da3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@gchanan gchanan gchanan requested changes

@colesbury colesbury colesbury approved these changes

Assignees

No one assigned

Labels

Merged module: autograd Related to torch.autograd, and the autograd engine in general module: docs Related to our documentation, both in docs/ and docblocks module: internals Related to internal abstractions in c10 and ATen module: onnx Related to torch.onnx module: pybind Related to our Python bindings / interactions with other Python libraries module: third_party

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@umanwizard @ezyang @facebook-github-bot @colesbury @gchanan @pytorchbot @mruberry