-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Add magic TORCH_MAKE_PYBIND_ENUM_FASTER macro #163527
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
base: gh/swolchok/841/base
Are you sure you want to change the base?
Conversation
See comment on the macro definition. In short, pybind11 3.x added `py::native_enum`, and also had to add overhead for that new way to bind enums on the critical path for calling functions that take regular old `py::enum_`s as arguments (for example, `__eq__`). Differential Revision: [D82873169](https://our.internmc.facebook.com/intern/diff/D82873169/) [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/163527
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 9685224 with merge base 98c4e35 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
See comment on the macro definition. In short, pybind11 3.x added `py::native_enum`, and also had to add overhead for that new way to bind enums on the critical path for calling functions that take regular old `py::enum_`s as arguments (for example, `__eq__`). Differential Revision: [D82873169](https://our.internmc.facebook.com/intern/diff/D82873169/) ghstack-source-id: 311049024 Pull Request resolved: #163527
See comment on the macro definition. In short, pybind11 3.x added `py::native_enum`, and also had to add overhead for that new way to bind enums on the critical path for calling functions that take regular old `py::enum_`s as arguments (for example, `__eq__`). Differential Revision: [D82873169](https://our.internmc.facebook.com/intern/diff/D82873169/) cc H-Huang awgu wanchaol fegin fduwjj wz337 wconstab d4l3k pragupta ezyang msaroufim dcci EikanWang jgong5 wenzhe-nrv sanchitintel voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx chenyang78 kadeng chauhang amjames Lucaskabela [ghstack-poisoned]
Pull Request resolved: #163527 See comment on the macro definition. In short, pybind11 3.x added `py::native_enum`, and also had to add overhead for that new way to bind enums on the critical path for calling functions that take regular old `py::enum_`s as arguments (for example, `__eq__`). ghstack-source-id: 311290067 @exported-using-ghexport Differential Revision: [D82873169](https://our.internmc.facebook.com/intern/diff/D82873169/)
Add a link to the pybind11 issue so we can remove this when it's resolved over there |
I haven't filed an issue and I don't see a particular reason to think it's structurally fixable. type_casters are keyed on the type they are casting, not on whether the pybind11 user registered the type with |
I maintain pybind11 so happy to discuss if there is a better way to optimize the casters or refactor it better in the long term |
See comment on the macro definition. In short, pybind11 3.x added `py::native_enum`, and also had to add overhead for that new way to bind enums on the critical path for calling functions that take regular old `py::enum_`s as arguments (for example, `__eq__`). Differential Revision: [D82873169](https://our.internmc.facebook.com/intern/diff/D82873169/) cc H-Huang awgu wanchaol fegin fduwjj wz337 wconstab d4l3k pragupta ezyang msaroufim dcci EikanWang jgong5 wenzhe-nrv sanchitintel voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx chenyang78 kadeng chauhang amjames Lucaskabela [ghstack-poisoned]
Pull Request resolved: #163527 See comment on the macro definition. In short, pybind11 3.x added `py::native_enum`, and also had to add overhead for that new way to bind enums on the critical path for calling functions that take regular old `py::enum_`s as arguments (for example, `__eq__`). ghstack-source-id: 311326172 @exported-using-ghexport Differential Revision: [D82873169](https://our.internmc.facebook.com/intern/diff/D82873169/)
See comment on the macro definition. In short, pybind11 3.x added `py::native_enum`, and also had to add overhead for that new way to bind enums on the critical path for calling functions that take regular old `py::enum_`s as arguments (for example, `__eq__`). Differential Revision: [D82873169](https://our.internmc.facebook.com/intern/diff/D82873169/) cc H-Huang awgu wanchaol fegin fduwjj wz337 wconstab d4l3k pragupta ezyang msaroufim dcci EikanWang jgong5 wenzhe-nrv sanchitintel voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx chenyang78 kadeng chauhang amjames Lucaskabela [ghstack-poisoned]
Pull Request resolved: #163527 See comment on the macro definition. In short, pybind11 3.x added `py::native_enum`, and also had to add overhead for that new way to bind enums on the critical path for calling functions that take regular old `py::enum_`s as arguments (for example, `__eq__`). ghstack-source-id: 311521197 @exported-using-ghexport Differential Revision: [D82873169](https://our.internmc.facebook.com/intern/diff/D82873169/)
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.
Approving, but this will be annoyingly difficult to get people to remember to do lol
I think it's straightforward to cause py::enum_ to break builds if this macro has not been used. Upstreaming something like this, together with optional support for said build breaks, is a possibility. |
will be bypassing test failures since the only new failure is actually a resource download error |
giving pull / linux-jammy-py3.13-clang12 a chance to rerun first |
See comment on the macro definition. In short, pybind11 3.x added `py::native_enum`, and also had to add overhead for that new way to bind enums on the critical path for calling functions that take regular old `py::enum_`s as arguments (for example, `__eq__`). Differential Revision: [D82873169](https://our.internmc.facebook.com/intern/diff/D82873169/) cc H-Huang awgu wanchaol fegin fduwjj wz337 wconstab d4l3k pragupta ezyang msaroufim dcci EikanWang jgong5 wenzhe-nrv sanchitintel voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx chenyang78 kadeng chauhang amjames Lucaskabela [ghstack-poisoned]
Pull Request resolved: #163527 See comment on the macro definition. In short, pybind11 3.x added `py::native_enum`, and also had to add overhead for that new way to bind enums on the critical path for calling functions that take regular old `py::enum_`s as arguments (for example, `__eq__`). ghstack-source-id: 312288551 @exported-using-ghexport Differential Revision: [D82873169](https://our.internmc.facebook.com/intern/diff/D82873169/)
@pytorchbot merge (Initiating merge automatically since Phabricator Diff has merged) |
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 |
See comment on the macro definition. In short, pybind11 3.x added `py::native_enum`, and also had to add overhead for that new way to bind enums on the critical path for calling functions that take regular old `py::enum_`s as arguments (for example, `__eq__`). Differential Revision: [D82873169](https://our.internmc.facebook.com/intern/diff/D82873169/) Pull Request resolved: #163527 Approved by: https://github.com/ezyang
@pytorchbot revert |
❌ 🤖 pytorchbot command failed:
Try |
@pytorchbot revert -m "breaking import torch in debug builds, see #164297" -c nosignal |
@pytorchbot successfully started a revert job. Check the current status here. |
@swolchok your PR has been successfully reverted. |
This reverts commit 50c0550. Reverted #163527 on behalf of https://github.com/swolchok due to breaking import torch in debug builds, see #164297 ([comment](#163527 (comment)))
Stack from ghstack (oldest at bottom):
See comment on the macro definition. In short, pybind11 3.x
added
py::native_enum
, and also had to add overhead for that new wayto bind enums on the critical path for calling functions that take
regular old
py::enum_
s as arguments (for example,__eq__
).Differential Revision: D82873169
cc @H-Huang @awgu @wanchaol @fegin @fduwjj @wz337 @wconstab @d4l3k @pragupta @ezyang @msaroufim @dcci @EikanWang @jgong5 @wenzhe-nrv @sanchitintel @voznesenskym @penguinwu @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @jiayisunx @chenyang78 @kadeng @chauhang @amjames @Lucaskabela