-
Notifications
You must be signed in to change notification settings - Fork 606
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
base: main
Are you sure you want to change the base?
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.
set(TORCH_CXXFLAGS "-D_GLIBCXX_USE_CXX11_ABI=${_use_cxx11_abi} -fabi-version=${_cxx_abi_version}") | ||
elseif(${CMAKE_CXX_COMPILER_ID} STREQUAL "Clang") | ||
set(TORCH_CXXFLAGS "-D_GLIBCXX_USE_CXX11_ABI=${_use_cxx11_abi} -U__GXX_ABI_VERSION -D__GXX_ABI_VERSION=10${_cxx_abi_version} '-DPYBIND11_COMPILER_TYPE=\"_gcc\"'") | ||
message(WARNING "Could not infer torch._C._PYBIND_BUILD_ABI. This was removed after pytorch updated to pybind11 version 3.0.1, and the TORCH_CXX_FLAGS manipulation is no longer required.") |
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.
For some reason I could not add this comment to lines 34-35 of this file -- I think we can modify the content on those lines to mention that after pybinf 3.0.1 upgrade setting the C++ ABI compatibility version is not required.
|
||
# Specialize compile flags for compiler | ||
if(${CMAKE_CXX_COMPILER_ID} STREQUAL "GNU") | ||
set(TORCH_CXXFLAGS "-D_GLIBCXX_USE_CXX11_ABI=${_use_cxx11_abi} -fabi-version=${_cxx_abi_version}") |
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.
Is it possible to not throw the warning on
torch-mlir/projects/CMakeLists.txt
Line 59 in 18e6b7f
message(WARNING |
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_FLAGS
whenever 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.