Skip to content

Conversation

bdhirsh
Copy link
Contributor

@bdhirsh bdhirsh commented Oct 14, 2022

This API adds some improvements to external backends who are building C++ backends out of tree using the PrivateUse1 dispatch key.

The docs and linked examples go over the API in more detail, but you should be able to use it like:

# This should probably be in the __init__.py file of a external backend's python package
> torch.register_privateuse1_backend("foo")`
# And it will allow the user to do this:
> a = torch.ones(2, device="foo")

Stack from ghstack (oldest at bottom):

@pytorch-bot
Copy link

pytorch-bot bot commented Oct 14, 2022

🔗 Helpful Links

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

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 Failures

As of commit 2fad0f2:

The following jobs have failed:

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

… names"

This API adds some improvements to external backends who are building C++ backends out of tree using the `PrivateUse1` dispatch key.

The docs and linked examples go over the API in more detail, but you should be able to use it like:
```
# This should probably be in the __init__.py file of a external backend's python package
> torch.register_privateuse1_backend("foo")`
# And it will allow the user to do this:
> a = torch.ones(2, device="foo")
```




[ghstack-poisoned]
… names"

This API adds some improvements to external backends who are building C++ backends out of tree using the `PrivateUse1` dispatch key.

The docs and linked examples go over the API in more detail, but you should be able to use it like:
```
# This should probably be in the __init__.py file of a external backend's python package
> torch.register_privateuse1_backend("foo")`
# And it will allow the user to do this:
> a = torch.ones(2, device="foo")
```




[ghstack-poisoned]
Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

Sounds good to me a couple questions about possible improvements.

)

add_docstr(
torch.register_privateuse1_backend,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure about the name here.
In particular, keeping in the back of my mind that we might need to extend this to support multiple backends.

I am wondering if we want to make an API that will support the multi backend case directly here. And thus naming it something like torch.register_new_backend("foo"). And then from c++ recommend they use getDispatchKeyForDeviceType(parse_type("foo")) (not sure what is the exact name for this?) to register their kernels.

The other option is to say they need to use PrivateUse1 manually and thus this API is just to rename it and much more limited. In such a case, I would suggest we move it to a smaller namespace like torch.utils.rename_privateuse1_backend("foo").

Neither of these options require changing the way things are implemented right now btw, just the python naming, the doc and the error message when multiple backends are being used.
WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And then from c++ recommend they use getDispatchKeyForDeviceType(parse_type("foo"))

I don't think this will easily work without some bigger API changes, since right now on the C++ side you still need to hardcode the PrivateUse1 key name directly into our registration macro: TORCH_LIBRARY_IMPL(aten, PrivateUse1, m) {...}.

If we wanted to fully go down the multi-backend route now (and abstract PrivateUse1 away from any public API's), one way would be with a new macro like TORCH_BACKEND_IMPL(aten, foo, m {...}) where we set up the link between a PrivateUse key and the backend name at library load time. That would make a python API like torch.register_new_backend() redundant though.

LMK if you agree with that line of reasoning - but I'm leaning towards your second proposal, which is to use a smaller namespace (torch.utils.rename_privateuse1_backend)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good!

… names"

This API adds some improvements to external backends who are building C++ backends out of tree using the `PrivateUse1` dispatch key.

The docs and linked examples go over the API in more detail, but you should be able to use it like:
```
# This should probably be in the __init__.py file of a external backend's python package
> torch.register_privateuse1_backend("foo")`
# And it will allow the user to do this:
> a = torch.ones(2, device="foo")
```




[ghstack-poisoned]
bdhirsh added a commit that referenced this pull request Oct 17, 2022
Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

SGTM
lint needs fixing though

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 17, 2022
… names"

This API adds some improvements to external backends who are building C++ backends out of tree using the `PrivateUse1` dispatch key.

The docs and linked examples go over the API in more detail, but you should be able to use it like:
```
# This should probably be in the __init__.py file of a external backend's python package
> torch.register_privateuse1_backend("foo")`
# And it will allow the user to do this:
> a = torch.ones(2, device="foo")
```




[ghstack-poisoned]
… names"

This API adds some improvements to external backends who are building C++ backends out of tree using the `PrivateUse1` dispatch key.

The docs and linked examples go over the API in more detail, but you should be able to use it like:
```
# This should probably be in the __init__.py file of a external backend's python package
> torch.register_privateuse1_backend("foo")`
# And it will allow the user to do this:
> a = torch.ones(2, device="foo")
```




[ghstack-poisoned]
bdhirsh added a commit that referenced this pull request Oct 17, 2022
… names"

This API adds some improvements to external backends who are building C++ backends out of tree using the `PrivateUse1` dispatch key.

The docs and linked examples go over the API in more detail, but you should be able to use it like:
```
# This should probably be in the __init__.py file of a external backend's python package
> torch.register_privateuse1_backend("foo")`
# And it will allow the user to do this:
> a = torch.ones(2, device="foo")
```




[ghstack-poisoned]
… names"

This API adds some improvements to external backends who are building C++ backends out of tree using the `PrivateUse1` dispatch key.

The docs and linked examples go over the API in more detail, but you should be able to use it like:
```
# This should probably be in the __init__.py file of a external backend's python package
> torch.register_privateuse1_backend("foo")`
# And it will allow the user to do this:
> a = torch.ones(2, device="foo")
```




[ghstack-poisoned]
bdhirsh added a commit that referenced this pull request Oct 18, 2022
@bdhirsh
Copy link
Contributor Author

bdhirsh commented Oct 19, 2022

@pytorchbot merge -f "flaky failure"

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

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

@github-actions
Copy link
Contributor

Hey @bdhirsh.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

@jeanschmidt
Copy link
Contributor

@pytorchbot revert -m "breaking internal builds - D40534212 - arstudio-windows-tests-landcastle-0" -c "ghfirst"

@jeanschmidt
Copy link
Contributor

Seems it is breaking windows builds:

stderr: xplat\caffe2\c10\core\DeviceType.cpp(139,38): error: no member named 'toupper' in namespace 'std'; did you mean simply 'toupper'?
   ->         [](unsigned char c) { return std::toupper(c); });
   ->                                      ^~~~~~~~~~~~
   ->                                      toupper
   -> C:/tools/toolchains/vs2017_15.9/WindowsSdk/Include/10.0.17763.0/ucrt\ctype.h(59,56): note: 'toupper' declared here
   -> Check_return
CRT_JIT_INTRINSIC _ACRTIMP int __cdecl toupper(_In int _C);
   ->                                                        ^
   -> xplat\caffe2\c10\core\DeviceType.cpp(157,36): error: no member named 'tolower' in namespace 'std'; did you mean simply 'tolower'?
   ->       [](unsigned char c) { return std::tolower(c); });
   ->                                    ^~~~~~~~~~~~
   ->                                    tolower
   -> C:/tools/toolchains/vs2017_15.9/WindowsSdk/Include/10.0.17763.0/ucrt\ctype.h(62,56): note: 'tolower' declared here
   -> Check_return
CRT_JIT_INTRINSIC _ACRTIMP int __cdecl tolower(_In int _C);
   ->                                                        ^

check toupper/tolower usage

@pytorchmergebot
Copy link
Collaborator

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

@pytorchmergebot
Copy link
Collaborator

@bdhirsh your PR has been successfully reverted.

pytorchmergebot added a commit that referenced this pull request Oct 20, 2022
…mes (#86992)"

This reverts commit fb6826b.

Reverted #86992 on behalf of https://github.com/jeanschmidt due to breaking internal builds - D40534212 - arstudio-windows-tests-landcastle-0
@bdhirsh bdhirsh reopened this Oct 21, 2022
bdhirsh added a commit that referenced this pull request Oct 21, 2022
…mes (#86992)"

This reverts commit a895af9.

ghstack-source-id: c2dbee8
Pull Request resolved: #87453
bdhirsh added a commit that referenced this pull request Oct 21, 2022
@bdhirsh
Copy link
Contributor Author

bdhirsh commented Oct 21, 2022

Closing, reland at #87453

@bdhirsh bdhirsh closed this Oct 21, 2022
pytorchmergebot pushed a commit that referenced this pull request Oct 21, 2022
kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Nov 5, 2022
kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Dec 10, 2022
@facebook-github-bot facebook-github-bot deleted the gh/bdhirsh/327/head branch June 8, 2023 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged Reverted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants