Add torch.nested namespace
#84102
Add torch.nested namespace
#84102
Conversation
[ghstack-poisoned]
🔗 Helpful links
✅ No Failures (5 Pending)As of commit 6d9f89f (more details on the Dr. CI page): Expand to see more💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
facebook-github-bot
added
the
oncall: jit
[ghstack-poisoned]
mikaylagawarecki
requested review from
bdhirsh,
bhosmer and
drisspg
and removed request for
anjali411 and
soulitzer
drisspg
added
module: nestedtensor
| +++++++++++++++++++++++++ | ||
|
|
||
| The following Tensor methods are related to nested tensors: | ||
| The following functions are related to nested tensors: |
There was a problem hiding this comment.
"The following functions are specific to nested tensors:" ?
There was a problem hiding this comment.
Hm, so these will be functions in the nested namespace, hence the change, but they still exist as methods. What do you think would be the best way of putting this across?
There was a problem hiding this comment.
I think that only functions should be here and methods will be documented on the Tensor object directly?
There was a problem hiding this comment.
Just to confirm, we still want to_padded_tensor to live as a function in the torch.nested namespace, is that correct?
There was a problem hiding this comment.
Alban suggested to me that we could potentially remove the method on the tensor base. And use the top level python api to create the method. This python level implementation would under the hood call the torch.nested.to_padded_tensor() function
But yeah I think as long as there is one function exposed in python not having both torch.to_padded_tensor(nt, 0) and torch.nested.to_padded_tensor(nt, 0 ) that is good.
test/test_nestedtensor.py
Outdated
|
|
||
| def test_nested_namespace(self): | ||
| nt = torch.nested_tensor([torch.randn(2, 3), torch.randn(4, 5)]) | ||
| result = nt.to_padded_tensor(4) |
There was a problem hiding this comment.
yeah I hope it isn't the pattern duplicate torch.* namespace functions into the aliases because having both feels really wrong
There was a problem hiding this comment.
This one is method-only. So there is nothing in torch.*.
There was a problem hiding this comment.
Ohh see when then added a function that goes in nested namespace? Do we need that? Should only functional variants go in nested namespace?
There was a problem hiding this comment.
In general yes because you don't want nested-specific methods on plain Tensor.
But I guess it's too late :p
| +++++++++++++++++++++++++ | ||
|
|
||
| The following Tensor methods are related to nested tensors: | ||
| The following functions are related to nested tensors: |
There was a problem hiding this comment.
I think that only functions should be here and methods will be documented on the Tensor object directly?
| "torch.multiprocessing.spawn": [ | ||
| "Optional" | ||
| ], | ||
| "torch.nested": [ |
There was a problem hiding this comment.
Why do you need this since you already have a __all__ there?
There was a problem hiding this comment.
TestCorrectModuleNames seemed to fail with just the __all__ before adding it here. I'll check again to make sure
There was a problem hiding this comment.
Double checked this, test_public_bindings.py:test_correct_module_names seems to fail without this
test/test_nestedtensor.py
Outdated
|
|
||
| def test_nested_namespace(self): | ||
| nt = torch.nested_tensor([torch.randn(2, 3), torch.randn(4, 5)]) | ||
| result = nt.to_padded_tensor(4) |
There was a problem hiding this comment.
This one is method-only. So there is nothing in torch.*.
First step towards #83775 - only `to_padded_tensor` is moved to the nested namespace for now - following the schema used for `special`, `fft`, `linalg` and other namespaces, nested functions are registered in native_functions.yaml as `nested_{function_name}` and are bound to the desired Python name in `torch/nested/__init__.py`, and the desired C++ name in `torch/csrc/api/include/torch/nested.h`. **Question**: should we keep the documentation for `Tensor.to_padded_tensor` or can this deleted since it is shared by `torch.nested.to_padded_tensor`? [generated nested docs](https://docs-preview.pytorch.org/84102/nested.html?highlight=nested#module-torch.nested) [ghstack-poisoned]
First step towards #83775 - only `to_padded_tensor` is moved to the nested namespace for now - following the schema used for `special`, `fft`, `linalg` and other namespaces, nested functions are registered in native_functions.yaml as `nested_{function_name}` and are bound to the desired Python name in `torch/nested/__init__.py`, and the desired C++ name in `torch/csrc/api/include/torch/nested.h`. **Question**: should we keep the documentation for `Tensor.to_padded_tensor` or can this deleted since it is shared by `torch.nested.to_padded_tensor`? [generated nested docs](https://docs-preview.pytorch.org/84102/nested.html?highlight=nested#module-torch.nested) [ghstack-poisoned]
mikaylagawarecki
removed request for
H-Huang,
awgu,
digantdesai,
dzdang,
jerryzh168,
jianyuh,
kimishpatel,
rohan-varma,
salilsdesai,
vkuzo,
z-a-f and
zhaojuanmao
|
@pytorchbot rebase -s |
|
@pytorchbot successfully started a rebase job. Check the current status here |
|
Tried to rebase and push PR #84102, but it was already up to date |
|
@pytorchbot rebase -s |
|
@pytorchbot successfully started a rebase job. Check the current status here |
First step towards #83775 - only `to_padded_tensor` is moved to the nested namespace for now - following the schema used for `special`, `fft`, `linalg` and other namespaces, nested functions are registered in native_functions.yaml as `nested_{function_name}` and are bound to the desired Python name in `torch/nested/__init__.py`, and the desired C++ name in `torch/csrc/api/include/torch/nested.h`. ~~**Question**: should we keep the documentation for `Tensor.to_padded_tensor` or can this deleted since it is shared by `torch.nested.to_padded_tensor`?~~ [generated nested docs](https://docs-preview.pytorch.org/84102/nested.html?highlight=nested#module-torch.nested) Differential Revision: [D39361148](https://our.internmc.facebook.com/intern/diff/D39361148) [ghstack-poisoned]
|
Successfully rebased |
|
@mikaylagawarecki has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@pytorchbot merge (Initiating merge automatically since Phabricator Diff has merged) |
|
@pytorchbot successfully started a merge job. Check the current status here. |
|
Hey @mikaylagawarecki. |
Summary: Pull Request resolved: #84102 First step towards #83775 - only `to_padded_tensor` is moved to the nested namespace for now - following the schema used for `special`, `fft`, `linalg` and other namespaces, nested functions are registered in native_functions.yaml as `nested_{function_name}` and are bound to the desired Python name in `torch/nested/__init__.py`, and the desired C++ name in `torch/csrc/api/include/torch/nested.h`. ~~**Question**: should we keep the documentation for `Tensor.to_padded_tensor` or can this deleted since it is shared by `torch.nested.to_padded_tensor`?~~ [generated nested docs](https://docs-preview.pytorch.org/84102/nested.html?highlight=nested#module-torch.nested) Test Plan: Imported from OSS Reviewed By: drisspg Differential Revision: D39361148 Pulled By: mikaylagawarecki fbshipit-source-id: add218c3e05a6922735b32c248adde4463dd3f4d
5 participants
First step towards #83775
to_padded_tensoris moved to the nested namespace for nowspecial,fft,linalgand other namespaces, nested functions are registered in native_functions.yaml asnested_{function_name}and are bound to the desired Python name intorch/nested/__init__.py, and the desired C++ name intorch/csrc/api/include/torch/nested.h.Question: should we keep the documentation forTensor.to_padded_tensoror can this deleted since it is shared bytorch.nested.to_padded_tensor?generated nested docs
Stack from ghstack (oldest at bottom):
torch.nestednamespace #84102Differential Revision: D39361148