-
Couldn't load subscription status.
- Fork 607
Handle pytorch pybind11 changes and bump nightly pins #4352
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
Conversation
The old path is still left in case we need to build against older versions of pytorch. Signed-off-by: zjgarvey <[email protected]>
Signed-off-by: zjgarvey <[email protected]>
Signed-off-by: zjgarvey <[email protected]>
Signed-off-by: zjgarvey <[email protected]>
Signed-off-by: zjgarvey <[email protected]>
|
cc: @raayandhar |
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.
LGTM, some minor nits. Thanks for fixing this issue.
|
Sorry this is taking a bit to land. I'm a bit skeptical about this approach, specifically because of the version difference for pybind11 between pytorch and us, so I'm looking a bit deeper into what is going on and trying to make sure this isn't just a lucky hack or something. |
|
Ah, it looks like we include pybind headers from torch instead of our own pybind pip install, which is why this ends up working. The only think I'd want to be sure of is the use of Edit: I think I'm wrong about the default libraries used by clang on linux. I think it actually defaults to |
This change also more accurately screens for when `torch._C` doesn't have the attribute `_PYBIND11_BUILD_ABI`. Signed-off-by: zjgarvey <[email protected]>
Signed-off-by: zjgarvey <[email protected]>
I think this is the simplest approach, for now, to resolve #4343
It would be good to eventually finish #4348 ; however, it became a bit too much to rework the generated sources scripts in a timely fashion.
See also another parallel attempt to address the ci problems: #4345
This PR modifies the Cmake pytorch configure function to simply not set any
TORCH_CXX_FLAGSwhenever pytorch is missing the old PYBIND_BUILD_ABI tag. I think whatever compiler flags we were pushing through to make pybind think we are GCC and to use a specific ABI version is just completely unnecessary now. I was worried we might need to update our pybind version in the requirements, but it appears to not be relevant.Additionally, nightly pins are updated and small fixes are made to resolve misc failures in tests after the bump.