Replace all RAIIATH with Tensor in libtorch_agnostic test, test some APIs #155977
Conversation
…APIs [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/155977
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 4c80667 with merge base 03488d8 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
… test some APIs" [ghstack-poisoned]
| return data_ptr; | ||
| } | ||
|
|
||
| int64_t dim() const { |
There was a problem hiding this comment.
Why are these signed instead of unsigned anyway...
There was a problem hiding this comment.
The same answer as many of these things in PyTorch: Historical reasons :D
I would mention in this case, that since all the function that do take a "dim" argument must be signed (to handle -1 dim etc), then it is more convenient to have the same type throughout for everything that represents "dim" and avoid casting from unsigned to signed everywhere.
| reinterpret_cast<float*>(param.data_ptr()), | ||
| reinterpret_cast<float*>(grad.data_ptr()), | ||
| reinterpret_cast<float*>(out.data_ptr()), |
There was a problem hiding this comment.
side thought: I feel like the templated data_ptr() accessors (with dtype check) would be nice to help users to get safer code (by not casting a float16 to float32 by mistake) and const correctness (to get the right COW behavior).
There was a problem hiding this comment.
As in for an API to add to stable::Tensor, right?
| Tensor res = sgd_out_of_place( | ||
| to<Tensor>(stack[0]), | ||
| to<Tensor>(stack[1]), | ||
| float(to<double>(stack[2])), |
There was a problem hiding this comment.
it doesn't have to be, i just defined it this way to show someone how they'd use float if their kernel takes float.
… test some APIs" [ghstack-poisoned]
… test some APIs" [ghstack-poisoned]
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
I realize I was passing stable::Tensors by value (thus making a copy every time) which is not what I want from the `from` function that converts Ts to StableIValues. `from` should not mutate the input and should be read-only. I asked an LLM whether this is API BC breaking (with an intuition that it shouldn't be), and it said no, cuz: 1. "Passing by const reference is more permissive than passing by value. e.g., if T is a type that has a deleted or inaccessible copy constructor (e.g., std::unique_ptr), the original code would have been invalid, while the new code would be valid." Nice. We are good with additive. 2. We didn't modify the original input before (cuz we took a copy) and we don't now (cuz we promise const). Update: The LLM failed to mention primitives, with which we should not pass references around, so we are only changing the signatures of std::optional<T> and stable::Tensor Pull Request resolved: #156126 Approved by: https://github.com/swolchok ghstack dependencies: #155367, #155977
4 participants
Stack from ghstack (oldest at bottom):