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

[PyTorch] Devirtualize TensorImpl::dim() with macro #49770

Closed
wants to merge 5 commits into from

Conversation

swolchok
Copy link
Contributor

@swolchok swolchok commented Dec 23, 2020

Stack from ghstack:

Seems like the performance cost of making this commonly-called method virtual isn't worth having use of undefined tensors crash a bit earlier (they'll still fail to dispatch).

Differential Revision: D25687465

NOTE FOR REVIEWERS: This PR has internal Facebook specific changes or comments, please review them on Phabricator!

Seems like the performance cost of making this commonly-called method virtual isn't worth having use of undefined tensors crash a bit earlier (they'll still fail to dispatch).

Differential Revision: [D25687465](https://our.internmc.facebook.com/intern/diff/D25687465/)

**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25687465/)!

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Dec 23, 2020

💊 CI failures summary and remediations

As of commit d7e578f (more details on the Dr. CI page):


  • 2/2 failures possibly* introduced in this PR
    • 1/2 non-CircleCI failure(s)

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

See CircleCI build pytorch_libtorch_linux_xenial_cuda11_1_cudnn8_py3_gcc7_build (1/1)

Step: "Build" (full log | diagnosis details | 🔁 rerun)

Jan 07 18:06:41 sccache: error: couldn't connect to server
Jan 07 18:06:41 +++ eval 'extract_trap_cmd '
Jan 07 18:06:41 ++++ extract_trap_cmd
Jan 07 18:06:41 ++++ printf '%s\n' ''
Jan 07 18:06:41 +++ printf '%s\n' cleanup
Jan 07 18:06:41 ++ trap -- '
Jan 07 18:06:41 cleanup' EXIT
Jan 07 18:06:41 ++ [[ pytorch-libtorch-linux-xenial-cuda11.1-cudnn8-py3-gcc7-build != *pytorch-win-* ]]
Jan 07 18:06:41 ++ which sccache
Jan 07 18:06:41 ++ sccache --stop-server
Jan 07 18:06:41 Stopping sccache server...
Jan 07 18:06:41 sccache: error: couldn't connect to server
Jan 07 18:06:41 sccache: caused by: Connection refused (os error 111)
Jan 07 18:06:41 ++ true
Jan 07 18:06:41 ++ rm /var/lib/jenkins/sccache_error.log
Jan 07 18:06:41 rm: cannot remove '/var/lib/jenkins/sccache_error.log': No such file or directory
Jan 07 18:06:41 ++ true
Jan 07 18:06:41 ++ [[ pytorch-libtorch-linux-xenial-cuda11.1-cudnn8-py3-gcc7-build == *rocm* ]]
Jan 07 18:06:41 ++ SCCACHE_ERROR_LOG=/var/lib/jenkins/sccache_error.log
Jan 07 18:06:41 ++ SCCACHE_IDLE_TIMEOUT=1200
Jan 07 18:06:41 ++ RUST_LOG=sccache::server=error
Jan 07 18:06:41 ++ sccache --start-server

1 job timed out:

  • pytorch_libtorch_linux_xenial_cuda11_1_cudnn8_py3_gcc7_build

This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

This comment has been revised 33 times.

swolchok added a commit that referenced this pull request Dec 23, 2020
Seems like the performance cost of making this commonly-called method virtual isn't worth having use of undefined tensors crash a bit earlier (they'll still fail to dispatch).

Differential Revision: [D25687465](https://our.internmc.facebook.com/intern/diff/D25687465/)

**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25687465/)!

ghstack-source-id: 119068428
Pull Request resolved: #49770
@swolchok swolchok requested review from ezyang and ngimel January 5, 2021 19:06
Seems like the performance cost of making this commonly-called method virtual isn't worth having use of undefined tensors crash a bit earlier (they'll still fail to dispatch).

Differential Revision: [D25687465](https://our.internmc.facebook.com/intern/diff/D25687465/)

**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25687465/)!

[ghstack-poisoned]
swolchok added a commit that referenced this pull request Jan 5, 2021
Pull Request resolved: #49770

Seems like the performance cost of making this commonly-called method virtual isn't worth having use of undefined tensors crash a bit earlier (they'll still fail to dispatch).
ghstack-source-id: 119411550

Differential Revision: [D25687465](https://our.internmc.facebook.com/intern/diff/D25687465/)

**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25687465/)!
Copy link
Contributor

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

OK. This change is a little hard to understand from the perspective of an OSS only user, since there is no point in the codebase where we ever actually set the macro. A Note explaining the situation would go a long way to explaining to people what's going on. (It's also worth pointing out XLA is the blocker for unconditionally removing the macro entirely)

Seems like the performance cost of making this commonly-called method virtual isn't worth having use of undefined tensors crash a bit earlier (they'll still fail to dispatch).

Differential Revision: [D25687465](https://our.internmc.facebook.com/intern/diff/D25687465/)

**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25687465/)!

[ghstack-poisoned]
swolchok added a commit that referenced this pull request Jan 6, 2021
Pull Request resolved: #49770

Seems like the performance cost of making this commonly-called method virtual isn't worth having use of undefined tensors crash a bit earlier (they'll still fail to dispatch).
ghstack-source-id: 119462149

Differential Revision: [D25687465](https://our.internmc.facebook.com/intern/diff/D25687465/)

**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25687465/)!
@swolchok
Copy link
Contributor Author

swolchok commented Jan 6, 2021

A Note explaining the situation

Added to the PR at the bottom of the stack that adds TENSORIMPL_MAYBE_VIRTUAL.

Seems like the performance cost of making this commonly-called method virtual isn't worth having use of undefined tensors crash a bit earlier (they'll still fail to dispatch).

Differential Revision: [D25687465](https://our.internmc.facebook.com/intern/diff/D25687465/)

**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25687465/)!

[ghstack-poisoned]
swolchok added a commit that referenced this pull request Jan 6, 2021
Pull Request resolved: #49770

Seems like the performance cost of making this commonly-called method virtual isn't worth having use of undefined tensors crash a bit earlier (they'll still fail to dispatch).
ghstack-source-id: 119477211

Differential Revision: [D25687465](https://our.internmc.facebook.com/intern/diff/D25687465/)

**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25687465/)!
@swolchok swolchok added the module: bc-breaking Related to a BC-breaking change label Jan 6, 2021
Seems like the performance cost of making this commonly-called method virtual isn't worth having use of undefined tensors crash a bit earlier (they'll still fail to dispatch).

Differential Revision: [D25687465](https://our.internmc.facebook.com/intern/diff/D25687465/)

**NOTE FOR REVIEWERS**: This PR has internal Facebook specific changes or comments, please review them on [Phabricator](https://our.internmc.facebook.com/intern/diff/D25687465/)!

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 4de6b27.

@facebook-github-bot
Copy link
Contributor

This pull request has been reverted by c215ffb.

@facebook-github-bot facebook-github-bot deleted the gh/swolchok/58/head branch January 11, 2021 15:17
hwangdeyu pushed a commit to hwangdeyu/pytorch that referenced this pull request Jan 14, 2021
Summary:
Pull Request resolved: pytorch#49770

Seems like the performance cost of making this commonly-called method virtual isn't worth having use of undefined tensors crash a bit earlier (they'll still fail to dispatch).
ghstack-source-id: 119528065

Test Plan: framework overhead benchmarks

Reviewed By: ezyang

Differential Revision: D25687465

fbshipit-source-id: 89aabce165a594be401979c04236114a6f527b59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants