Add the __torch_function__ API override mechanism#30730
Add the __torch_function__ API override mechanism#30730ngoldbaum wants to merge 1 commit intopytorch:masterfrom
Conversation
ngoldbaum
force-pushed
the
torch_function_redo
branch
from
1b9ea84 to
0b52a11
Compare
| # smaller chance that enabling this test will land simultaneously | ||
| # with a new operator being added to the torch namespace. | ||
|
|
||
| # assert len(untested_funcs) == 0, msg.format(pprint.pformat(untested_funcs)) |
There was a problem hiding this comment.
The commented out test and explanatory comment are here.
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
The |
|
@shoyer I didn't consider that, because I'm quite used to code moving around between NumPy, SciPy and other projects, and we normally don't bother with formal copyright notices. The And a note for completeness, due credit was given of course, this is the note at the top of |
|
No worries, and I did notice those credits :) I wasn't sure what license pytorch uses, but given that it appears to be 3 clause BSD like NumPy this is probably fine. (I've been trained to be a little overly sensitive on this stuff at Google, where we're asked to add copyright notices to every file in our own open source projects.) |
Yeah, that never makes much sense to me. Files change over time, and usually any copyright notice that's put in at some point in time becomes outdated. The one way to figure it out is by looking at the per-line git history after some time of evolution.
Yes, same license as NumPy. Okay thanks, I take that as no PR needed. Although I did just check https://github.com/pytorch/pytorch/blob/master/LICENSE#L33 which says so it's a few years out of date, may update that anyway at least:) |
|
Yeah, our LICENSE file is super out of date. Updates more than welcome. |
|
Hi @ngoldbaum, we found there is some performance regression introduced from your PR. According to our benchmark, we found where before means current codebase, and after means remove the change in this PR. To reproduce the benchmark, please read https://github.com/pytorch/pytorch/tree/master/benchmarks/operator_benchmark and run |
|
@ngoldbaum This is probably related to the decorator hooks. Maybe this function needs to get moved to C++? |
|
I believe that regression is expected given that #27064 (comment) If we don't want to have overhead for the operators that are implemented in Python then I think we would need to rewrite the |
|
@ngoldbaum I think I made a good argument that we could get it pretty fast with proper decorator design: just a few direct type() comparisons in straightline bytecode. @hl475 is the operator above the only regressing one or are there others you caught in your test suite? Let's either (1) remove the decorator from them or (2) port them to C++. If doing (2) turns out to be difficult in some cases, let's reopen the question of a fancier decorator mechanism. |
|
@ezyang I think PyTorch/Caffe2 Operator Micro-benchmarks |
|
A 2.3 us hit is anyway unexpected, that's way more than what a single use of |
I'm not sure what happened here. It looks like the PR is open now, but the merge was not reverted. Should we close this again and open a new issue about this performance regression? |
Yes, unless you think we should revert the entire diff. (but if it's just decorator related, I'd prefer if we just delete the decorators) |
|
Yeah don't think a full revert is going to be helpful. Let's just try and figure this out in the next couple of hours first. I'll close this and open a new issue, will Cc you all. |
|
@rgommers -- while I have you here, is there a discussion anywhere of why the protocol for |
Summary: This is a re-do of pytorch#27064, which was reverted (pytorch@b8792c0). This was landed at the same time as other work that added new operators to the `torch` namespace so the check for whether the `torch` namespace is exhaustively checked for overridability was triggering test failures. I've temporarily disabled that check and added an explanatory comment that the check will be re-enabled in a future PR that will be merged during a time when the commit velocity on PyTorch is lower. Pull Request resolved: pytorch#30730 Differential Revision: D18813270 Pulled By: ezyang fbshipit-source-id: 70477c4656dca8fea6e7bc59259555041fcfbf68
@shoyer sorry for the long delay in answering this. I forgot the answer, and never got around to digging through the history of when I did this around Sep'19. Now the discussion at pytorch/rfcs#3 reminded me: I believe I removed (or never added) Also note that in In pytorch/rfcs#3 there'll be some more follow-up that may result in changes. |
|
@rgommers thanks for following up here! It has been really interesting to follow along the work your team has been doing in supporting overrides in PyTorch. I'm impressed by how you've been able to eliminate almost all of the overhead! I asked because the my motivation for the |
…onal (#32799) Summary: This adds `__torch_function__` support for all functions in `torch.functional` and `torch.nn.functional`. The changes to C++ code and codegen scripts are to facilitate adding `__torch_function__` support for the native functions in `torch._C._nn`. Note that I moved the `handle_torch_function` C++ function to a header that both `python_torch_functions.cpp` and `python_nn_functions.cpp` include. The changes to `python_nn_functions.cpp` mirror the changes I made to `python_torch_functions.cpp` when `__torch_function__` support was first added in #27064. Due to the somewhat different way the `torch._C` and `torch._C._nn` namespaces are initialized I needed to create a new static reference to the `torch._C._nn` namespace (`THPNNVariableFunctions`). I'm not sure if that is the best way to do this. In principle I could import these namespaces in each kernel and avoid the global variable but that would have a runtime cost. I added `__torch_function__` support to the Python functions in `torch.nn.functional` following the approach in #32194. I re-enabled the test that checks if all functions in the `torch` namespace are explicitly tested for `__torch_function__` support. I also generalized the check to work for `torch.functional` and `torch.nn.functional` as well. This test was explicitly disabled in #30730 and I'm happy to disable it again if you think that's appropriate. I figured now was as good a time as any to try to re-enable it. Finally I adjusted the existing torch API tests to suppress deprecation warnings and add keyword arguments used by some of the code in `torch.nn.functional` that were missed when I originally added the tests in #27064. Pull Request resolved: #32799 Differential Revision: D19956809 Pulled By: ezyang fbshipit-source-id: 40d34e0109cc4b9f3ef62f409d2d35a1d84e3d22
…onal (pytorch#32799) Summary: This adds `__torch_function__` support for all functions in `torch.functional` and `torch.nn.functional`. The changes to C++ code and codegen scripts are to facilitate adding `__torch_function__` support for the native functions in `torch._C._nn`. Note that I moved the `handle_torch_function` C++ function to a header that both `python_torch_functions.cpp` and `python_nn_functions.cpp` include. The changes to `python_nn_functions.cpp` mirror the changes I made to `python_torch_functions.cpp` when `__torch_function__` support was first added in pytorch#27064. Due to the somewhat different way the `torch._C` and `torch._C._nn` namespaces are initialized I needed to create a new static reference to the `torch._C._nn` namespace (`THPNNVariableFunctions`). I'm not sure if that is the best way to do this. In principle I could import these namespaces in each kernel and avoid the global variable but that would have a runtime cost. I added `__torch_function__` support to the Python functions in `torch.nn.functional` following the approach in pytorch#32194. I re-enabled the test that checks if all functions in the `torch` namespace are explicitly tested for `__torch_function__` support. I also generalized the check to work for `torch.functional` and `torch.nn.functional` as well. This test was explicitly disabled in pytorch#30730 and I'm happy to disable it again if you think that's appropriate. I figured now was as good a time as any to try to re-enable it. Finally I adjusted the existing torch API tests to suppress deprecation warnings and add keyword arguments used by some of the code in `torch.nn.functional` that were missed when I originally added the tests in pytorch#27064. Pull Request resolved: pytorch#32799 Differential Revision: D19956809 Pulled By: ezyang fbshipit-source-id: 40d34e0109cc4b9f3ef62f409d2d35a1d84e3d22
Summary: According to pytorch/rfcs#3 From the goals in the RFC: 1. Support subclassing `torch.Tensor` in Python (done here) 2. Preserve `torch.Tensor` subclasses when calling `torch` functions on them (done here) 3. Use the PyTorch API with `torch.Tensor`-like objects that are _not_ `torch.Tensor` subclasses (done in #30730) 4. Preserve `torch.Tensor` subclasses when calling `torch.Tensor` methods. (done here) 5. Propagating subclass instances correctly also with operators, using views/slices/indexing/etc. (done here) 6. Preserve subclass attributes when using methods or views/slices/indexing. (done here) 7. A way to insert code that operates on both functions and methods uniformly (so we can write a single function that overrides all operators). (done here) 8. The ability to give external libraries a way to also define functions/methods that follow the `__torch_function__` protocol. (will be addressed in a separate PR) This PR makes the following changes: 1. Adds the `self` argument to the arg parser. 2. Dispatches on `self` as well if `self` is not `nullptr`. 3. Adds a `torch._C.DisableTorchFunction` context manager to disable `__torch_function__`. 4. Adds a `torch::torch_function_enabled()` and `torch._C._torch_function_enabled()` to check the state of `__torch_function__`. 5. Dispatches all `torch._C.TensorBase` and `torch.Tensor` methods via `__torch_function__`. TODO: - [x] Sequence Methods - [x] Docs - [x] Tests Closes #28361 Benchmarks in #37091 (comment) Pull Request resolved: #37091 Reviewed By: ngimel Differential Revision: D22765678 Pulled By: ezyang fbshipit-source-id: 53f8aa17ddb8b1108c0997f6a7aa13cb5be73de0
6 participants
This is a re-do of #27064, which was reverted (b8792c0). This was landed at the same time as other work that added new operators to the
torchnamespace so the check for whether thetorchnamespace is exhaustively checked for overridability was triggering test failures.I've temporarily disabled that check and added an explanatory comment that the check will be re-enabled in a future PR that will be merged during a time when the commit velocity on PyTorch is lower.