Update custom Function preserve torch function when inputs returned as-is #109825

soulitzer · 2023-09-21T21:07:20Z

Stack from ghstack (oldest at bottom):

-> Update custom Function preserve torch function when inputs returned as-is #109825

Fixes #109805

…s-is [ghstack-poisoned]

pytorch-bot · 2023-09-21T21:07:22Z

🔗 Helpful Links

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

📄 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 b184a54 with merge base 255d1a7 ():
💚 Looks good so far! There are no failures yet. 💚

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

…s-is ghstack-source-id: ca3dc2e Pull Request resolved: #109825

soulitzer · 2023-09-21T21:10:24Z

torch/csrc/autograd/python_function.cpp

  };

+  auto view_as_fn = [](const at::Tensor& x, const at::Tensor& y) -> at::Tensor {
+    pybind11::gil_scoped_acquire gil;


I'm not actually sure we need this since we are calling from Python and we don't release the gil, but jvp_user_function does it.

torch/csrc/autograd/custom_function.h

albanD · 2023-09-26T11:44:11Z

torch/csrc/autograd/python_function.cpp

+    pybind11::gil_scoped_acquire gil;
+    py::object py_x = py::cast(x);
+    py::function py_view_as_method = py_x.attr("view_as");
+    return py_view_as_method(y).cast<at::Tensor>();


Could you do a quick local check with %timeit to make sure this is not making this case significantly slower please?
If it does, we should use our direct APIs via THPVariable_wrap and cie to avoid the pybind overhead.

I did a quick test, and it seems that this PR adds 0.3-4 us of overhead, but avoiding use of pybind can cut 0.1 us of that away.

Current: 2.66 us

Without pybind: 2.95 us

With pybind 3.05 us

I've updated this PR to avoid using pybind to avoid that overhead.

… returned as-is" Fixes #109805 [ghstack-poisoned]

…s-is ghstack-source-id: 72b3101 Pull Request resolved: #109825

albanD

SGTM!

soulitzer · 2023-10-02T21:06:38Z

@pytorchbot merge

pytorchmergebot · 2023-10-02T21:08:27Z

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 · 2023-10-02T21:08:36Z

Merge failed

Reason: 1 mandatory check(s) failed. The first few are:

pull / linux-focal-cuda11.8-py3.10-gcc9 / test (distributed, 1, 3, linux.8xlarge.nvidia.gpu)

Dig deeper by viewing the failures on hud

Details for Dev Infra team

Raised by workflow job

Failing merge rule: Core Maintainers

soulitzer · 2023-10-03T17:01:46Z

Failures look legit, going to investigate.

… returned as-is" Fixes #109805 [ghstack-poisoned]

soulitzer · 2023-10-04T15:01:17Z

Failures look legit, going to investigate.

This failures is a expected result of the bug fix. The failure occurs in a test that tests that DDP properly fails when used with LazyModule. The difference is now that we now fail earlier with a different error message.

When DDP tries to replicate the module it calls Broadcast, which is a custom Function that returns input as-is. LazyModules have parameters of instance UninitializedTensorMixin which has a __torch_function__ method that errors if that unitialized parameter is used. With this PR we no longer incorrectly bypass this check, leading to a different error to be observed in the test.

… returned as-is" Fixes #109805 [ghstack-poisoned]

…s-is ghstack-source-id: bfbdb86 Pull Request resolved: #109825

soulitzer · 2023-10-04T22:42:25Z

@pytorchbot merge

pytorchmergebot · 2023-10-04T22:44:49Z

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

PaliC · 2023-10-05T23:47:58Z

@pytorchbot revert -m "causing a plethora of internal failures" -c ghfirst

pytorchmergebot · 2023-10-05T23:49:35Z

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

pytorchmergebot · 2023-10-05T23:49:47Z

@soulitzer your PR has been successfully reverted.

…turned as-is (#109825)" This reverts commit 4e73eee. Reverted #109825 on behalf of https://github.com/PaliC due to causing a plethora of internal failures ([comment](#109825 (comment)))

…r… (pytorch#110679) Summary: …eturned as-is reland of pytorch#109825 (comment) Opening this without ghstack to do codev. In our PR, we changed the signature of `_wrap_outputs`. There is some internal code that calls `_wrap_outputs` directly, so we also need to update that callsite. Differential Revision: D49983654 Pulled By: soulitzer

#110679) …eturned as-is reland of #109825 (comment) Opening this without ghstack to do codev. In our PR, we changed the signature of `_wrap_outputs`. There is some internal code that calls `_wrap_outputs` directly, so we also need to update that callsite. Pull Request resolved: #110679 Approved by: https://github.com/albanD

…r… (pytorch#110679) Summary: …eturned as-is reland of pytorch#109825 (comment) Opening this without ghstack to do codev. In our PR, we changed the signature of `_wrap_outputs`. There is some internal code that calls `_wrap_outputs` directly, so we also need to update that callsite. Differential Revision: D49983654 Pulled By: soulitzer

Update custom Function preserve torch function when inputs returned a…

653e26c

…s-is [ghstack-poisoned]

soulitzer requested a review from albanD as a code owner September 21, 2023 21:07

soulitzer added a commit that referenced this pull request Sep 21, 2023

Update custom Function preserve torch function when inputs returned a…

6a3b830

…s-is ghstack-source-id: ca3dc2e Pull Request resolved: #109825

soulitzer commented Sep 21, 2023

View reviewed changes

albanD reviewed Sep 26, 2023

View reviewed changes

Update on "Update custom Function preserve torch function when inputs…

aba2695

… returned as-is" Fixes #109805 [ghstack-poisoned]

soulitzer added a commit that referenced this pull request Oct 1, 2023

Update custom Function preserve torch function when inputs returned a…

1420440

…s-is ghstack-source-id: 72b3101 Pull Request resolved: #109825

soulitzer requested a review from albanD October 1, 2023 18:02

albanD approved these changes Oct 2, 2023

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 2, 2023

soulitzer added release notes: autograd release notes category topic: bug fixes topic category labels Oct 2, 2023

pytorchmergebot added the merging label Oct 2, 2023

pytorchmergebot removed the merging label Oct 2, 2023

Update on "Update custom Function preserve torch function when inputs…

d29fdc2

… returned as-is" Fixes #109805 [ghstack-poisoned]

soulitzer requested review from H-Huang, awgu, fduwjj, fegin, kwen2501, mrshenli, rohan-varma, wanchaol and zhaojuanmao as code owners October 4, 2023 14:54

soulitzer requested a review from wz337 as a code owner October 4, 2023 14:54

Update on "Update custom Function preserve torch function when inputs…

b184a54

… returned as-is" Fixes #109805 [ghstack-poisoned]

soulitzer added a commit that referenced this pull request Oct 4, 2023

Update custom Function preserve torch function when inputs returned a…

fbc8931

…s-is ghstack-source-id: bfbdb86 Pull Request resolved: #109825

pytorchmergebot added the merging label Oct 4, 2023

pytorchmergebot added Merged and removed merging labels Oct 4, 2023

pytorchmergebot closed this in 4e73eee Oct 4, 2023

pytorchmergebot added the Reverted label Oct 5, 2023

facebook-github-bot deleted the gh/soulitzer/234/head branch October 8, 2023 14:23

Update custom Function preserve torch function when inputs returned as-is #109825

Update custom Function preserve torch function when inputs returned as-is #109825

Uh oh!

Conversation

soulitzer commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

✅ No Failures

Uh oh!

soulitzer Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

albanD Sep 26, 2023

Choose a reason for hiding this comment

Uh oh!

soulitzer Oct 1, 2023

Choose a reason for hiding this comment

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

soulitzer commented Oct 2, 2023

Uh oh!

pytorchmergebot commented Oct 2, 2023

Merge started

Uh oh!

pytorchmergebot commented Oct 2, 2023

Merge failed

Uh oh!

soulitzer commented Oct 3, 2023

Uh oh!

soulitzer commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soulitzer commented Oct 4, 2023

Uh oh!

pytorchmergebot commented Oct 4, 2023

Merge started

Uh oh!

PaliC commented Oct 5, 2023

Uh oh!

pytorchmergebot commented Oct 5, 2023

Uh oh!

pytorchmergebot commented Oct 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

soulitzer commented Sep 21, 2023 •

edited

Loading

pytorch-bot bot commented Sep 21, 2023 •

edited

Loading

soulitzer commented Oct 4, 2023 •

edited

Loading