Extract DeviceType to a standalone header file #152787
Conversation
Summary: Move c10/core/DeviceType.h to a separate torch/csrc/header_only directory. Still keep a copy of c10/core/DeviceType.h for backwrad compatibility. More header files will be moved as follow-up. CI to guard "header-only-ness" will be added later. [ghstack-poisoned]
Summary: Move c10/core/DeviceType.h to a separate torch/csrc/header_only directory. Still keep a copy of c10/core/DeviceType.h for backwrad compatibility. More header files will be moved as follow-up. CI to guard "header-only-ness" will be added later. ghstack-source-id: da9c987 Pull Request resolved: #152787
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/152787
Note: Links to docs will display an error until the docs builds have been completed. ❌ 4 New Failures, 1 Unrelated FailureAs of commit dcd35f2 with merge base 533fc58 ( NEW FAILURES - The following jobs have failed:
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. |
Summary: Move c10/core/DeviceType.h to a separate torch/csrc/header_only directory. Still keep a copy of c10/core/DeviceType.h for backwrad compatibility. More header files will be moved as follow-up. CI to guard "header-only-ness" will be added later. cc EikanWang jgong5 wenzhe-nrv sanchitintel mingfeima XiaobingSuper ashokei jingxu10 jerryzh168 gujinghui PenghuiCheng jianyuh min-jean-cho yanbing-j Guobing-Chen Xia-Weiwen snadampal [ghstack-poisoned]
Summary: Move c10/core/DeviceType.h to a separate torch/csrc/header_only directory. Still keep a copy of c10/core/DeviceType.h for backwrad compatibility. More header files will be moved as follow-up. CI to guard "header-only-ness" will be added later. ghstack-source-id: 78dc65a Pull Request resolved: #152787
Summary: Move c10/core/DeviceType.h to a separate torch/csrc/header_only directory. Still keep a copy of c10/core/DeviceType.h for backwrad compatibility. More header files will be moved as follow-up. CI to guard "header-only-ness" will be added later. cc EikanWang jgong5 wenzhe-nrv sanchitintel mingfeima XiaobingSuper ashokei jingxu10 jerryzh168 gujinghui PenghuiCheng jianyuh min-jean-cho yanbing-j Guobing-Chen Xia-Weiwen snadampal [ghstack-poisoned]
Summary: Move c10/core/DeviceType.h to a separate torch/csrc/header_only directory. Still keep a copy of c10/core/DeviceType.h for backwrad compatibility. More header files will be moved as follow-up. CI to guard "header-only-ness" will be added later. ghstack-source-id: fd59c97 Pull Request resolved: #152787
|
I dont have context or opinion on this but probably best to leave some reason for the move in the summary |
Summary: Move c10/core/DeviceType.h to a separate torch/csrc/header_only directory. Still keep a copy of c10/core/DeviceType.h for backwrad compatibility. More header files will be moved as follow-up. CI to guard "header-only-ness" will be added later. cc EikanWang jgong5 wenzhe-nrv sanchitintel mingfeima XiaobingSuper ashokei jingxu10 jerryzh168 gujinghui PenghuiCheng jianyuh min-jean-cho yanbing-j Guobing-Chen Xia-Weiwen snadampal [ghstack-poisoned]
Summary: Move c10/core/DeviceType.h to a separate torch/csrc/header_only directory. Still keep a copy of c10/core/DeviceType.h for backwrad compatibility. More header files will be moved as follow-up. CI to guard "header-only-ness" will be added later. ghstack-source-id: 234322c Pull Request resolved: #152787
|
@desertfire has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: Move c10/core/DeviceType.h to a separate torch/csrc/header_only directory. Still keep a copy of c10/core/DeviceType.h for backwrad compatibility. More header files will be moved as follow-up. CI to guard "header-only-ness" will be added later. ghstack-source-id: 43b2d2b Pull Request resolved: #152787
Summary: Move c10/core/DeviceType.h to a separate torch/csrc/header_only directory. Still keep a copy of c10/core/DeviceType.h for backwrad compatibility. More header files will be moved as follow-up. CI to guard "header-only-ness" will be added later. cc EikanWang jgong5 wenzhe-nrv sanchitintel mingfeima XiaobingSuper ashokei jingxu10 jerryzh168 gujinghui PenghuiCheng jianyuh min-jean-cho yanbing-j Guobing-Chen Xia-Weiwen snadampal Differential Revision: [D74184681](https://our.internmc.facebook.com/intern/diff/D74184681) [ghstack-poisoned]
Summary: Move c10/core/DeviceType.h to a separate torch/csrc/header_only directory. Still keep a copy of c10/core/DeviceType.h for backwrad compatibility. More header files will be moved as follow-up. CI to guard "header-only-ness" will be added later. ghstack-source-id: 88686b6 Pull Request resolved: #152787
Summary: Move c10/core/DeviceType.h to a separate torch/csrc/header_only directory. Still keep a copy of c10/core/DeviceType.h for backwrad compatibility. More header files will be moved as follow-up. CI to guard "header-only-ness" will be added later. cc EikanWang jgong5 wenzhe-nrv sanchitintel mingfeima XiaobingSuper ashokei jingxu10 jerryzh168 gujinghui PenghuiCheng jianyuh min-jean-cho yanbing-j Guobing-Chen Xia-Weiwen snadampal Differential Revision: [D74184681](https://our.internmc.facebook.com/intern/diff/D74184681) [ghstack-poisoned]
Summary: Move c10/core/DeviceType.h to a separate torch/csrc/header_only directory. Still keep a copy of c10/core/DeviceType.h for backwrad compatibility. More header files will be moved as follow-up. CI to guard "header-only-ness" will be added later. ghstack-source-id: f415bf8 Pull Request resolved: #152787
|
@desertfire has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: Move c10/core/DeviceType.h to a separate torch/csrc/header_only directory. Still keep a copy of c10/core/DeviceType.h for backwrad compatibility. More header files will be moved as follow-up. CI to guard "header-only-ness" will be added later. cc EikanWang jgong5 wenzhe-nrv sanchitintel mingfeima XiaobingSuper ashokei jingxu10 jerryzh168 gujinghui PenghuiCheng jianyuh min-jean-cho yanbing-j Guobing-Chen Xia-Weiwen snadampal Differential Revision: [D74184681](https://our.internmc.facebook.com/intern/diff/D74184681) [ghstack-poisoned]
Summary: Move c10/core/DeviceType.h to a separate torch/csrc/header_only directory. Still keep a copy of c10/core/DeviceType.h for backwrad compatibility. More header files will be moved as follow-up. CI to guard "header-only-ness" will be added later. ghstack-source-id: 861a96d Pull Request resolved: #152787
|
@desertfire has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| // it doesn't work for fbobjc build, error: declaration requires a global | ||
| // destructor C10_EXPORT is also needed to make sure the variables are | ||
| // visible to all files. | ||
| inline C10_EXPORT std::atomic<bool>& privateuse1_backend_name_set() { |
There was a problem hiding this comment.
To reviewers: most function bodies in this file are a straight-forward copy-paste from DeviceType.cpp, except the following three functions which are specially crafted to satisfy the global variable linking requirements on different platforms.
There was a problem hiding this comment.
What exactly are the global variable linking requirements on diff platforms and how do we make sure they are all still satisfied after this change?
There was a problem hiding this comment.
There were build-time errors on internal and OSS CIs. One of them is a fbobjc build error as I wrote in the comment. Another is a Windows linker error, "unresolved external symbol", which I solved by using C10_EXPORT.
There was a problem hiding this comment.
All the changes apart from the private_use_1 ones look great. I'm curious about what @albanD has to say about those bits.
I agree with keeping the c10:: namespace to minimize migration friction as much as possible.
To confirm my understanding, does this mean that the only change to use a future-guaranteed header only API is to switch the include from:
#include <c10/core/DeviceType.h>
to
#include <torch/standalone/header_only/core/DeviceType.h>
After talking to @swolchok, @albanD and @malfet, I've concluded we should start with manual testing of APIs we care about--could you add something similar to your existing tests where you have a file using the DeviceType APIs you want to keep standalone? That way, I'll take your tests and apply a more strict CI guarantee that the APIs we care about will be denoted somewhere and verified through manual tests.
(More context: while some automation is possible, not every symbol is discoverable through nm after clang -c and compiler based tools like find-all-symbols do not supply the schemas of functions so figuring that out amidst our many directives will take some time/may not be worth just manually denoting which APIs we want to maintain.)
|
Upon further discussion with @albanD, I'm revising my viewpoint about the c10:: namespace-ness a little bit--we should have the API be exposed in c10 (for the reasons you said) and also start a |
| } | ||
|
|
||
| inline C10_EXPORT std::string& privateuse1_backend_name() { | ||
| static std::string privateuse1_backend_name_; |
There was a problem hiding this comment.
Aren't static variables in headers a big no-no as it makes it local per-compilation unit which is very much not what we want here?
There was a problem hiding this comment.
inline removes your concern. If we declared the function as static or inline static, then yes, it won't work as you mentioned. https://stackoverflow.com/questions/185624/what-happens-to-static-variables-in-inline-functions
There was a problem hiding this comment.
This explanation is quite confusing to me. My read is that it will work within a single compilation unit but not across compilation units?
In particular, if I have this function inlined in libtorch.so and another version inlined in libuser.so. You're saying that the compiler will figure out a way to make them shared between the two? Which one of the two .so will actually host the shared variable? I'm quite confused how this can work.
There was a problem hiding this comment.
I think it's not the compiler but the linker which will only keep one copy of the symbol. The only alternative I can think of is to keep some .cpp files under the standalone directory. Let me know what do you think of that approach.
There was a problem hiding this comment.
As an example, with my change, nm shows privateuse1_backend_name is of type u:
"u" The symbol is a unique global symbol. This is a GNU extension to the standard set of ELF symbol bindings. For such a symbol the dynamic linker will make sure that in the
entire process there is just one symbol with this name and type in use.
There was a problem hiding this comment.
Ho that's pretty cool!
Given it's a GNU extension, will this work on clang and msvc as well?
There was a problem hiding this comment.
Given it's a GNU extension,
As per the stack overflow answer earlier in this thread, the standard requires that "A static local variable in an extern inline function always refers to the same object." A particular platform choosing to use platform-specific things to implement this requirement does not change that the requirement is coming from the standard.
Actually I started with introducing a new torch::standalone namespace. It mostly worked, except for a few annoying internal build, see one example here, https://www.internalfb.com/sandcastle/workflow/981784718774334552 . Basically there is a type name collision on |
|
How are we drawing the line on what to put in headers? "Let's just move everything from our .cpp files into headers" doesn't sound like a good idea; we normally don't do that for a reason. |
See also my recent musings on this issue from the ExecuTorch perspective in pytorch/executorch#8924 (comment) and the following couple comments. |
Summary: The goal of this PR and future follow-up PRs is to group a set of header files required by AOTInductor Standalone in a separate directory, ensuring they are implemented in a header-only manner. More specifically, here is what this PR does: * Extract the DeviceType enum class into a standalone header file in a new torch/standalone/header_only directory * Retain the existing c10/core/DeviceType.[h|cpp] files to handle complex logic and static variables * Import symbols from the new torch::standalone namespace into c10 for backward compatibility Differential Revision: [D74184681](https://our.internmc.facebook.com/intern/diff/D74184681) [ghstack-poisoned]
Summary: The goal of this PR and future follow-up PRs is to group a set of header files required by AOTInductor Standalone in a separate directory, ensuring they are implemented in a header-only manner. More specifically, here is what this PR does: * Extract the DeviceType enum class into a standalone header file in a new torch/standalone/header_only directory * Retain the existing c10/core/DeviceType.[h|cpp] files to handle complex logic and static variables * Import symbols from the new torch::standalone namespace into c10 for backward compatibility ghstack-source-id: 439eaaf Pull Request resolved: #152787
|
@desertfire has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: The goal of this PR and future follow-up PRs is to group a set of header files required by AOTInductor Standalone in a separate directory, ensuring they are implemented in a header-only manner. More specifically, here is what this PR does: * Extract the DeviceType enum class into a standalone header file in a new torch/standalone/header_only directory * Retain the existing c10/core/DeviceType.[h|cpp] files to handle complex logic and static variables * Import symbols from the new torch::standalone namespace into c10 for backward compatibility Differential Revision: [D74184681](https://our.internmc.facebook.com/intern/diff/D74184681) [ghstack-poisoned]
Summary: The goal of this PR and future follow-up PRs is to group a set of header files required by AOTInductor Standalone in a separate directory, ensuring they are implemented in a header-only manner. More specifically, here is what this PR does: * Extract the DeviceType enum class into a standalone header file in a new torch/standalone/header_only directory * Retain the existing c10/core/DeviceType.[h|cpp] files to handle complex logic and static variables * Import symbols from the new torch::standalone namespace into c10 for backward compatibility ghstack-source-id: 72b95c5 Pull Request resolved: #152787
|
@desertfire has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Stack from ghstack (oldest at bottom):
Summary: The goal of this PR and future follow-up PRs is to group a set of header files required by AOTInductor Standalone in a separate directory, ensuring they are implemented in a header-only manner. More specifically, here is what this PR does:
Differential Revision: D74184681