Update torch::stable::Tensor() default constructor #159507
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/159507
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New FailureAs of commit 5b28d27 with merge base f33ce40 ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
Allows things like
```cpp
Tensor cu_seqlens_q;
if (...) {
cu_seqlens_q = ...
}
...
```
[ghstack-poisoned]
| return size; | ||
| } | ||
|
|
||
| bool defined() const { |
There was a problem hiding this comment.
This is the same semantic as the one in TensorBase.h right?
And to make sure I'm understanding correctly: an undefined tensor is an uninitialized tensor which I know has no memory but is it basically a nullptr?
There was a problem hiding this comment.
Yes to the first
pytorch/torch/csrc/inductor/aoti_torch/shim_common.cpp
Lines 759 to 764 in 5f5f508
pytorch/aten/src/ATen/templates/TensorBody.h
Line 103 in 5f5f508
pytorch/aten/src/ATen/core/TensorBase.h
Line 95 in 5f5f508
My understanding is TensorBase() = default constructor would initialize its c10::intrusive_ptr<TensorImpl, UndefinedTensorImpl> impl_ to a nullptr indeed
I think there is a special case where UndefinedTensorImpl is not a nullptr but has defined its bool() method return False when defined() is called, but we're not using that here.
test/cpp_extensions/libtorch_agnostic_extension/libtorch_agnostic/ops.py
Outdated
Show resolved
Hide resolved
test/cpp_extensions/libtorch_agnostic_extension/libtorch_agnostic/ops.py
Outdated
Show resolved
Hide resolved
test/cpp_extensions/libtorch_agnostic_extension/test/test_libtorch_agnostic.py
Outdated
Show resolved
Hide resolved
test/cpp_extensions/libtorch_agnostic_extension/test/test_libtorch_agnostic.py
Outdated
Show resolved
Hide resolved
Allows things like
```cpp
Tensor cu_seqlens_q;
if (...) {
cu_seqlens_q = ...
}
...
```
Also adds `torch::stable::Tensor.defined()`
[ghstack-poisoned]
| libtorch_agnostic.ops.test_default_constructor(False) | ||
|
|
||
| final_memory = process.memory_info().rss | ||
| memory_increase = final_memory - initial_memory |
There was a problem hiding this comment.
woah a way to test CPU memory! how confident are you that this test is testing the right thing? This looks okay to me but I am not an expert
There was a problem hiding this comment.
I tried creating a list of tensors on CPU and the reserved set size increased, so I felt somewhat confident this was doing the right thing
cc @albanD do you know :P
There was a problem hiding this comment.
This will most likely be flaky as we don't control what else might grab cpu memory.
I would recommend running this test on cuda and check for cuda memory which is very reliable for this kind of things.
Don't forget to add @serialTest() decorator to avoid cross polution of course ;)
There was a problem hiding this comment.
This test is testing that the following code doesn't memory leak
stable::Tensor a;I don't think we can make this use any cuda memory unfortunately, should we just remove the test? @janeyx99
Allows things like
```cpp
Tensor cu_seqlens_q;
if (...) {
cu_seqlens_q = ...
}
...
```
Also adds `torch::stable::Tensor.defined()`
[ghstack-poisoned]
Allows things like
```cpp
Tensor cu_seqlens_q;
if (...) {
cu_seqlens_q = ...
}
...
```
Also adds `torch::stable::Tensor.defined()`
[ghstack-poisoned]
|
Starting merge as part of PR stack under #159328 |
1 similar comment
|
Starting merge as part of PR stack under #159328 |
ghstack-source-id: 869d35c Pull Request resolved: pytorch#159507
|
Starting merge as part of PR stack under #159328 |
1 similar comment
|
Starting merge as part of PR stack under #159328 |
|
Rebased |
|
Starting merge as part of PR stack under #159328 |
2 similar comments
|
Starting merge as part of PR stack under #159328 |
|
Starting merge as part of PR stack under #159328 |
Pull Request resolved: #159328 Approved by: https://github.com/janeyx99 ghstack dependencies: #159507
Allows things like
```cpp
Tensor cu_seqlens_q;
if (...) {
cu_seqlens_q = ...
}
...
```
Also adds `torch::stable::Tensor.defined()`
Pull Request resolved: #159507
Approved by: https://github.com/janeyx99
Pull Request resolved: #159328 Approved by: https://github.com/janeyx99 ghstack dependencies: #159507
Allows things like
```cpp
Tensor cu_seqlens_q;
if (...) {
cu_seqlens_q = ...
}
...
```
Also adds `torch::stable::Tensor.defined()`
Pull Request resolved: #159507
Approved by: https://github.com/janeyx99
Pull Request resolved: #159328 Approved by: https://github.com/janeyx99 ghstack dependencies: #159507
Allows things like
```cpp
Tensor cu_seqlens_q;
if (...) {
cu_seqlens_q = ...
}
...
```
Also adds `torch::stable::Tensor.defined()`
Pull Request resolved: pytorch#159507
Approved by: https://github.com/janeyx99
Pull Request resolved: pytorch#159328 Approved by: https://github.com/janeyx99 ghstack dependencies: pytorch#159507
Allows things like
```cpp
Tensor cu_seqlens_q;
if (...) {
cu_seqlens_q = ...
}
...
```
Also adds `torch::stable::Tensor.defined()`
Pull Request resolved: pytorch#159507
Approved by: https://github.com/janeyx99
Pull Request resolved: pytorch#159328 Approved by: https://github.com/janeyx99 ghstack dependencies: pytorch#159507
4 participants
Allows things like
Tensor cu_seqlens_q; if (...) { cu_seqlens_q = ... } ...Also adds
torch::stable::Tensor.defined()Stack from ghstack (oldest at bottom):