-
Notifications
You must be signed in to change notification settings - Fork 25.8k
Move static from_ivalue/to_ivalue to new shim_common.cpp #166373
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move static from_ivalue/to_ivalue to new shim_common.cpp #166373
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/166373
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ✅ No FailuresAs of commit 3541c2f with merge base 4295a9a ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
[ghstack-poisoned]
[ghstack-poisoned]
| "torch/csrc/inductor/aoti_torch/oss_proxy_executor.cpp", | ||
| "torch/csrc/inductor/inductor_ops.cpp", | ||
| "torch/csrc/jit/serialization/pickle.cpp", | ||
| "torch/csrc/shim_common.cpp", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this actually go in core_sources_common? or core_sources_full instead? since this doesn't have to do with inductor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had put it in core_sources_common previously and imported, but that caused all the mobile builds to fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmmmmmmmmmmmmmmmmmmmmmmm.
i think it may be important to figure out why that is 😛 otherwise this is a bit weird why it needs to be in inductor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the reason why is that this file includes the aoti shim (whose cpp file is only in the inductor sources)
So if we put this in core_sources_common, it would be included by the mobile builds (specifically this line), but then the aoti shim would be excluded
Separately, this new shim_common.cpp now implements some of aoti's shim.h, so I believe it's needed here
Lines 426 to 428 in 5e47b4d
| if(USE_LITE_AOTI) | |
| append_filelist("inductor_core_resources" LIBTORCH_CMAKE_SRCS) | |
| endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still not sure I fully understand if this is the right move, but given that it seems ok + that this is undoable, I am approving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks fine to me, barring two questions
[ghstack-poisoned]
Move `from_ivalue` and `to_ivalue` and their dependents `StableIValueBoxedKernel`, `aoti_torch_library_impl` `aoti_torch_call_dispatcher` into new (non-aoti shim_common.cpp) This is in prep for the above PRs where I add v2s (`torch_call_dispatcher` and `torch_library_impl`) that are versioning aware [ghstack-poisoned]
|
Starting merge as part of PR stack under #164332 |
…63683) Part 1 of plan in https://docs.google.com/document/d/1MaX51H5aEQE5XnOlnZIpf9oCYwzGrTWkgBACxNzsmWE/edit?usp=sharing - Upgrade `aoti_torch_call_dispatcher` to v2 with an `extension_build_version` - Allow registration of StableIValue stack --> IValue stack adapters for schema changes #### Note: This PR does not include a linter that tells the user to add the upgrader if the schema changes, which is an important piece that will be added in a separate PR Pull Request resolved: #163683 Approved by: https://github.com/janeyx99 ghstack dependencies: #164356, #166373
1. Add `extension_build_version` and `is_internal` to `FromImpl`/`ToImpl` (this will be useful for future if we need to break the BC of any type) #163832 has the PoC of how we would actually use this system 2. Add `aoti_torch_library_impl_v2` that takes in an additional `extension_build_version` argument, updates callsite in `torch/csrc/stable/library.h` to always pass `TORCH_ABI_VERSION` for this argument 3. Add `extension_build_version` to `from_ivalue` and `to_ivalue` and update all callsites 4. Add a private `_from` and `_to` that pass `is_internal=True` to `FromImpl`/`ToImpl`, making it easier to reason about what is being called from libtorch-land / extension-land **Note: This PR does not include a linter that tells the user to update from/to if changing the ABI of a type in headeronly, which I intend to do in #163998 Pull Request resolved: #164332 Approved by: https://github.com/janeyx99 ghstack dependencies: #164356, #166373, #163683
ghstack-source-id: a8e165e Pull Request resolved: pytorch#166373
Move `from_ivalue` and `to_ivalue` and their dependents `StableIValueBoxedKernel`, `aoti_torch_library_impl` `aoti_torch_call_dispatcher` into new (non-aoti shim_common.cpp) This is in prep for the above PRs where I add v2s (`torch_call_dispatcher` and `torch_library_impl`) that are versioning aware Pull Request resolved: #166373 Approved by: https://github.com/janeyx99 ghstack dependencies: #164356
…63683) Part 1 of plan in https://docs.google.com/document/d/1MaX51H5aEQE5XnOlnZIpf9oCYwzGrTWkgBACxNzsmWE/edit?usp=sharing - Upgrade `aoti_torch_call_dispatcher` to v2 with an `extension_build_version` - Allow registration of StableIValue stack --> IValue stack adapters for schema changes #### Note: This PR does not include a linter that tells the user to add the upgrader if the schema changes, which is an important piece that will be added in a separate PR Pull Request resolved: #163683 Approved by: https://github.com/janeyx99 ghstack dependencies: #164356, #166373
1. Add `extension_build_version` and `is_internal` to `FromImpl`/`ToImpl` (this will be useful for future if we need to break the BC of any type) #163832 has the PoC of how we would actually use this system 2. Add `aoti_torch_library_impl_v2` that takes in an additional `extension_build_version` argument, updates callsite in `torch/csrc/stable/library.h` to always pass `TORCH_ABI_VERSION` for this argument 3. Add `extension_build_version` to `from_ivalue` and `to_ivalue` and update all callsites 4. Add a private `_from` and `_to` that pass `is_internal=True` to `FromImpl`/`ToImpl`, making it easier to reason about what is being called from libtorch-land / extension-land **Note: This PR does not include a linter that tells the user to update from/to if changing the ABI of a type in headeronly, which I intend to do in #163998 Pull Request resolved: #164332 Approved by: https://github.com/janeyx99 ghstack dependencies: #164356, #166373, #163683
Move
from_ivalueandto_ivalueand their dependentsStableIValueBoxedKernel,aoti_torch_library_implaoti_torch_call_dispatcherinto new (non-aoti shim_common.cpp)This is in prep for the above PRs where I add v2s (
torch_call_dispatcherandtorch_library_impl) that are versioning awareStack from ghstack (oldest at bottom):