Move MemoryFormat/Layout to headeronly #168034
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/168034
Note: Links to docs will display an error until the docs builds have been completed. ⏳ 1 Pending, 1 Unrelated FailureAs of commit 32730de with merge base 1c04a43 ( 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]
| // Specialization for c10::Layout => StableIValue | ||
| // Note that we call into the shim to translate between the user's | ||
| // Layout and libtorch's Layout, which can be different! | ||
| using c10::Layout; |
There was a problem hiding this comment.
for these two I don't error in the base case bcos per "BC" they should use the base case in 2.9 (wdyt of that haha)
torch/headeronly/core/Layout.h
Outdated
| // Note: We can't use TORCH_CHECK here as it's not header-only | ||
| // Callers should ensure valid layout values | ||
| return stream << "Unknown"; |
There was a problem hiding this comment.
I'll fix this and below
There was a problem hiding this comment.
Should be okay to just replace with STD_TORCH_CHECK
There was a problem hiding this comment.
Actually....since that changes semantics...maybe an alternative is to just not migrate the >> functions
torch/headeronly/core/Layout.h
Outdated
| // Note: We can't use TORCH_CHECK here as it's not header-only | ||
| // Callers should ensure valid layout values | ||
| return stream << "Unknown"; |
There was a problem hiding this comment.
Should be okay to just replace with STD_TORCH_CHECK
janeyx99
changed the title
…HECK" [ghstack-poisoned]
janeyx99
changed the title
janeyx99
changed the title
~This PR does change the semantics of the >> operator by using STD_TORCH_CHECK to throw the error instead of TORCH_CHECK. Jane (who is writing this message) thinks it is okay because it is the error case when an invalid MemoryFormat or Layout is getting passed into >>, so the UX benefits of TORCH_CHECK over STD_TORCH_CHECK there are not significant enough to warrant making a new copy of Layout and MemoryFormat's >> APIs.~ Never mind! We shouldn't change TORCH_CHECK to STD_TORCH_CHECK for core usage ever, cuz the traceback info and c10::Error is very much desired!! So the solution is to not migrate the >>s. I pushed new commits to the stack to remove the >> code, but for reference, 8a30179 has all the code that I ended up deleting. [ghstack-poisoned]
janeyx99
added
release notes: cpp
|
@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 |
Merge failedReason: 1 jobs have failed, first few of them are: trunk / libtorch-linux-jammy-cuda12.8-py3.10-gcc11-debug / build Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge -i |
Merge startedYour change will be merged while ignoring the following 1 checks: trunk / libtorch-linux-jammy-cuda12.8-py3.10-gcc11-debug / build Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
This PR does change the semantics of the >> operator by using STD_TORCH_CHECK to throw the error instead of TORCH_CHECK. Jane (who is writing this message) thinks it is okay because it is the error case when an invalid MemoryFormat or Layout is getting passed into >>, so the UX benefits of TORCH_CHECK over STD_TORCH_CHECK there are not significant enough to warrant making a new copy of Layout and MemoryFormat's >> APIs.Never mind! We shouldn't change TORCH_CHECK to STD_TORCH_CHECK for core usage ever, cuz the traceback info and c10::Error is very much desired!! So the solution is to not migrate the >>s. I pushed new commits to the stack to remove the >> code, but for reference, 8a30179 has all the code that I ended up deleting.
Stack from ghstack (oldest at bottom):