Refactor out headeronly ArrayRef#164991
Refactor out headeronly ArrayRef#164991janeyx99 wants to merge 8 commits intogh/janeyx99/314/basefrom
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/164991
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 2 Unrelated FailuresAs of commit e5ab10c with merge base d7e2d0a ( NEW FAILURE - The following job has failed:
BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
[ghstack-poisoned]
[ghstack-poisoned]
| /// value. | ||
| /// | ||
| /// NOTE: We have refactored out the headeronly parts of the ArrayRef struct | ||
| /// into HOArrayRef. As adding `virtual` will change the performance of the |
| void debugCheckNullptrInvariant() { | ||
| TORCH_INTERNAL_ASSERT_DEBUG_ONLY( | ||
| Data != nullptr || Length == 0, | ||
| this->Data != nullptr || this->Length == 0, | ||
| "created ArrayRef with nullptr and non-zero length! std::optional relies on this being illegal"); | ||
| } |
There was a problem hiding this comment.
you can delete this function. "std::optional relies on this being illegal" is false. Relevant history here:
- [PyTorch] RFC: optimize c10::optional<ArrayRef<T>> #59333 (by yours truly) added this debug check, together with a specialization of c10::optionalc10::ArrayRef to reduce its size.
- [PyTorch] Redirect c10::optional to std::optional #101995 (by yours truly) removed the c10::optional and replaced it with a simple redirect to std::optional, "intentionally dropping the c10::optionalc10::ArrayRef size optimization" (with a note that it wasn't as important anymore). I forgot that this debug check was here, oops! sorry.
- [codemod]
c10:optional->std::optional#126135 find/replaced this message from c10::optional to std::optional.
I haven't specifically checked whether someone has tried to reintroduce this optimization, but if they had it would be undefined behavior -- you can't specialize std-namespaced things unless they explicitly allow you to, and I don't see any mention of specialization on https://en.cppreference.com/w/cpp/utility/optional.html .
if this check was important, I would instead ask you to guarantee it in HOArrayRef as well.
After you delete this function, you can get rid of all the constructor implementations that just call into HOArrayRef's constructors and simply write using HOArrayRef::HOArrayRef.
| } | ||
|
|
||
| /// front - Get the first element. | ||
| /// We deviate from HOArrayRef by using TORCH_CHECK instead of STD_TORCH_CHECK |
There was a problem hiding this comment.
is STD_TORCH_CHECK really so bad? if so, why is it always correct to call HOArrayRef functions instead? Is there some set of unwritten rules people have to be aware of about when it's OK for them to use HOArrayRef?
There was a problem hiding this comment.
From what I understand, c10::Error does some traceback handling to present the error in a nice way to the user. STD_TORCH_CHECK just calls std::runtime_error so it's less full. STD_TORCH_CHECK still checks properly though, but it scores fewer points in "user understandability", so it's not "incorrect", but not as ideal.
There was a problem hiding this comment.
How do you propose avoiding the creation of a perceived mess where people don't understand when they should use HeaderOnlyArrayRef vs ArrayRef?
There was a problem hiding this comment.
I'm now wondering if we can get STD_TORCH_CHECK to do the right thing -- throw c10::Error when linking with libtorch, but throw std::runtime_error when not linking with libtorch. I suspect we might be able to get the job done with weak symbols. Let me see if I can get a PR up...
There was a problem hiding this comment.
weak symbols will do the job on macOS (and presumably Linux), but Googling seems to suggest that there isn't an equivalent for Windows.
There was a problem hiding this comment.
I'm now wondering if we can get STD_TORCH_CHECK to do the right thing
per offline discussion, 1) @janeyx99 is concerned about code in headeronly changing its behavior depending on whether libtorch is linked 2) it is definitely unclear how to do this for windows, and we care about windows. therefore it is not clear that the behavior I proposed is the "right thing", shelving it.
| // nullptr Data and nonzero length in the following constructors: from a pointer | ||
| // and length, from a range, from a templated Container with .data() and .size(). | ||
| // HOArrayRef does not make that check for any constructor. | ||
| /// 2. ArrayRef can be constructed from a SmallVector. HOArrayRef cannot. |
There was a problem hiding this comment.
nitpick: you can still make a HOArrayRef given a SmallVector, you just don't get the convenience constructor
[ghstack-poisoned]
[ghstack-poisoned]
| /// into HeaderOnlyArrayRef. As adding `virtual` would change the performance of | ||
| /// the underlying constexpr calls, we rely on apparent-type dispatch for | ||
| /// inheritance. This should be fine because their memory format is the same, | ||
| /// and it is never incorrect for ArrayRef to call HeaderOnlyArrayRef methods. |
There was a problem hiding this comment.
I think we should probably add something like "However, you should prefer to use ArrayRef when possible, because its use of TORCH_CHECK will lead to better user-facing error messages."
| /// 1. ArrayRef has an extra convenience constructor for SmallVector. | ||
| /// 2. ArrayRef uses TORCH_CHECK. HeaderOnlyArrayRef uses header-only | ||
| /// STD_TORCH_CHECK, | ||
| /// which will output a std::runtime_error vs a c10::Error. |
There was a problem hiding this comment.
and here, something like "Consequently, you should use ArrayRef when possible and HeaderOnlyArrayRef only when necessary to support headeronly code."
| : Data(data), Length(length) { | ||
| debugCheckNullptrInvariant(); | ||
| } | ||
| // Inherits all constructors from HeaderOnlyArrayRef<T> |
There was a problem hiding this comment.
I don't love comments that explain C++ language features
|
|
||
| /// slice(n, m) - Take M elements of the array starting at element N | ||
| constexpr HeaderOnlyArrayRef<T> slice(size_t N, size_t M) const { | ||
| STD_TORCH_CHECK( |
There was a problem hiding this comment.
it occurs to me that having to repeat these error messages isn't great. if you wanted to cut that out, you could
#define ARRAY_REF_SLICE(check, class_name, N, M) \
check( \
N + M <= this->size(), \
// ...
#class_name ": invalid slice, N = ", \
// ...
up to you whether the macro ugliness is worth the deduplication, though.
[ghstack-poisoned]
|
Starting merge as part of PR stack under #165152 |
1 similar comment
|
Starting merge as part of PR stack under #165152 |
Pull Request resolved: #165152 Approved by: https://github.com/mikaylagawarecki ghstack dependencies: #164991
|
@pytorchbot revert -m "breaking internal tests D84961075" -c ghfirst |
|
@pytorchbot successfully started a revert job. Check the current status here. |
…165152)" This reverts commit e445494. Reverted #165152 on behalf of https://github.com/clee2000 due to breaking internal tests D84961075 ([comment](#164991 (comment)))
This reverts commit 3806e97. Reverted #164991 on behalf of https://github.com/clee2000 due to breaking internal tests D84961075 ([comment](#164991 (comment)))
|
@janeyx99 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@janeyx99 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Pull Request resolved: pytorch#164991 Approved by: https://github.com/swolchok
…165152) Pull Request resolved: pytorch#165152 Approved by: https://github.com/mikaylagawarecki ghstack dependencies: pytorch#164991
…ytorch#165152)" This reverts commit e445494. Reverted pytorch#165152 on behalf of https://github.com/clee2000 due to breaking internal tests D84961075 ([comment](pytorch#164991 (comment)))
This reverts commit 3806e97. Reverted pytorch#164991 on behalf of https://github.com/clee2000 due to breaking internal tests D84961075 ([comment](pytorch#164991 (comment)))
Pull Request resolved: pytorch#164991 Approved by: https://github.com/swolchok
…165152) Pull Request resolved: pytorch#165152 Approved by: https://github.com/mikaylagawarecki ghstack dependencies: pytorch#164991
…ytorch#165152)" This reverts commit e445494. Reverted pytorch#165152 on behalf of https://github.com/clee2000 due to breaking internal tests D84961075 ([comment](pytorch#164991 (comment)))
This reverts commit 3806e97. Reverted pytorch#164991 on behalf of https://github.com/clee2000 due to breaking internal tests D84961075 ([comment](pytorch#164991 (comment)))
Differential Revision: [D85091961](https://our.internmc.facebook.com/intern/diff/D85091961) [ghstack-poisoned]
|
Starting merge as part of PR stack under #165152 |
1 similar comment
|
Starting merge as part of PR stack under #165152 |
Pull Request resolved: #165152 Approved by: https://github.com/mikaylagawarecki ghstack dependencies: #164991
Pull Request resolved: #165153 Approved by: https://github.com/mikaylagawarecki ghstack dependencies: #164991, #165152
Some important notes: a) Just like IValues steal the ownership of ArrayRefs and any std::vectors in order to convert the inner elements into IValues, we do the same thing with StableIValue. This O(N) traverse is ineluctable. b) As a result, since StableIValues are owning and our contract is that to<T>(StableIValue) transfers ownership, you cannot ever convert from StableIValue to a nonowning HeaderOnlyArrayRef<V>. We handle memory similar to AtenTensorHandle, but we have a StableListHandle! Pull Request resolved: #165953 Approved by: https://github.com/malfet ghstack dependencies: #164991, #165152, #165153
This PR moves the implementations of Tensor accessor classes to headeronly with the following modifications:
- Add ArrayRef and IndexBoundsCheck template parameters to refactor out the usages of `IntArrayRef` and `TORCH_CHECK_INDEX` from Tensor accessor implementations.
- Eliminate usage of `c10::irange` as it is not headeronly-compatible.
- Introduce `torch::headeronly::{TensorAccessorBase,TensorAccessor, GenericPackedTensorAccessorBase, GenericPackedTensorAccessor}` that are headeronly-equivalent to `at::{TensorAccessorBase,TensorAccessor, GenericPackedTensorAccessorBase, GenericPackedTensorAccessor}`. Both these sets of template classes use original implementations from `torch::headeronly::detail` that have new template parameters `ArrayRefCls` and `IndexBoundsCheck` to facilitate `at` and `torch::headeronly` implementations of ArrayRef and checking indices.
TODO:
- ~when #164991 lands, eliminate the placeholder class HeaderOnlyArrayRef~ UPDATE: done.
Pull Request resolved: #166855
Approved by: https://github.com/janeyx99
ghstack-source-id: 7b6f852 Pull Request resolved: pytorch/pytorch#164991
Some important notes: a) Just like IValues steal the ownership of ArrayRefs and any std::vectors in order to convert the inner elements into IValues, we do the same thing with StableIValue. This O(N) traverse is ineluctable. b) As a result, since StableIValues are owning and our contract is that to<T>(StableIValue) transfers ownership, you cannot ever convert from StableIValue to a nonowning HeaderOnlyArrayRef<V>. We handle memory similar to AtenTensorHandle, but we have a StableListHandle! Pull Request resolved: pytorch#165953 Approved by: https://github.com/malfet ghstack dependencies: pytorch#164991, pytorch#165152, pytorch#165153
) Pull Request resolved: pytorch#167126 Approved by: https://github.com/Skylion007 ghstack dependencies: pytorch#164991, pytorch#165152, pytorch#165153, pytorch#165953
This PR moves the implementations of Tensor accessor classes to headeronly with the following modifications:
- Add ArrayRef and IndexBoundsCheck template parameters to refactor out the usages of `IntArrayRef` and `TORCH_CHECK_INDEX` from Tensor accessor implementations.
- Eliminate usage of `c10::irange` as it is not headeronly-compatible.
- Introduce `torch::headeronly::{TensorAccessorBase,TensorAccessor, GenericPackedTensorAccessorBase, GenericPackedTensorAccessor}` that are headeronly-equivalent to `at::{TensorAccessorBase,TensorAccessor, GenericPackedTensorAccessorBase, GenericPackedTensorAccessor}`. Both these sets of template classes use original implementations from `torch::headeronly::detail` that have new template parameters `ArrayRefCls` and `IndexBoundsCheck` to facilitate `at` and `torch::headeronly` implementations of ArrayRef and checking indices.
TODO:
- ~when pytorch#164991 lands, eliminate the placeholder class HeaderOnlyArrayRef~ UPDATE: done.
Pull Request resolved: pytorch#166855
Approved by: https://github.com/janeyx99
Stack from ghstack (oldest at bottom):
Differential Revision: D85091961