Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/meta-pytorch/torchcodec/1225
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit acb56db with merge base 8248f19 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
| if (!(cond)) { \ | ||
| throw std::out_of_range(std::string(msg)); \ | ||
| } \ | ||
| } while (false) |
There was a problem hiding this comment.
We previously used TORCH_CHECK_INDEX, but there is no stable equivalent (yet?). So until torch adds one (which we could probably do), we have to create our own. Slightly annoyingly, this goes back on a recent change we made in #1185 where we explicitly tried to stop using C++ exceptions. But that's not too bad. This is one of the many concessions we'll have to make in order to rely on the stable ABI.
Note that the weird do/while is normal, this is one of the common way to ensure that a multi-line macro will work as expected in all possible context. You'll find similar uses e.g. in https://github.com/pytorch/pytorch/blob/efa8f2943dd19bcc2e9223ed0afbba28ba19ffe7/c10/util/Exception.h#L382-L393
There was a problem hiding this comment.
If STABLE_CHECK_INDEX were implemented, we would not need to create this file at all, and we would directly include torch/csrc/stable/library.h in each file that needs STD_TORCH_CHECK or STABLE_CHECK_INDEX, right?
Adding this file is not a big change, but it is potentially a reason why we would implement STABLE_CHECK_INDEX ourselves.
There was a problem hiding this comment.
Thanks for the review! We'd still need to have this new file eventually, because we'll need some more helpers down the line. You can see the one in #1188 which has a fair amount of (shallow!) helpers.
My suggestion would be to implement most helpers ourselves so that we can move forward faster, and once everything is done, we can consider upstreaming stuff to core. I suggest this order for 2 reasons:
- upstreaming ops take a fair amount of time - we need to write it, then we need to go through review cycles, then we need to wait for it to be merged and for the nightlies to be available.
- we have no guarantees the ops we want to upstream will be accepted by core. Allow fmt headers in downstream libs with TORCH_STABLE_ONLY pytorch/pytorch#174372 is an example which, from internal discussions, is probably not going to make the cut.
First of a potentially long series.
The goal is to make TC rely on torch's stable ABI. This PR is simple: we use
STD_TORCH_CHECKinstead of the non-ABI-stableTORCH_CHECK.