-
Notifications
You must be signed in to change notification settings - Fork 24.9k
[OpenReg] Add OSX/Windows Support for OpenReg #159441
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
As the title stated. - Abstract platform-specific APIs - Add OSX/Windows support Co-authored-by: can-gaa-hou <jiahaochen535@gmail.com>
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/159441
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: ✅ You can merge normally! (1 Unrelated Failure)As of commit 65f2cc0 with merge base 846ada4 ( 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. |
test/cpp_extensions/open_registration_extension/torch_openreg/csrc/CMakeLists.txt
Outdated
Show resolved
Hide resolved
test/cpp_extensions/open_registration_extension/torch_openreg/csrc/CMakeLists.txt
Outdated
Show resolved
Hide resolved
test/cpp_extensions/open_registration_extension/torch_openreg/csrc/runtime/OpenRegFunctions.cpp
Outdated
Show resolved
Hide resolved
test/cpp_extensions/open_registration_extension/torch_openreg/csrc/runtime/OpenRegFunctions.cpp
Outdated
Show resolved
Hide resolved
test/cpp_extensions/open_registration_extension/torch_openreg/csrc/runtime/OpenRegFunctions.h
Outdated
Show resolved
Hide resolved
test/cpp_extensions/open_registration_extension/torch_openreg/csrc/runtime/OpenRegFunctions.h
Outdated
Show resolved
Hide resolved
test/cpp_extensions/open_registration_extension/torch_openreg/setup.py
Outdated
Show resolved
Hide resolved
test/cpp_extensions/open_registration_extension/torch_openreg/setup.py
Outdated
Show resolved
Hide resolved
.../cpp_extensions/open_registration_extension/torch_openreg/third_party/openreg/CMakeLists.txt
Outdated
Show resolved
Hide resolved
.../cpp_extensions/open_registration_extension/torch_openreg/third_party/openreg/CMakeLists.txt
Outdated
Show resolved
Hide resolved
test/cpp_extensions/open_registration_extension/torch_openreg/torch_openreg/__init__.py
Show resolved
Hide resolved
test/cpp_extensions/open_registration_extension/torch_openreg/torch_openreg/csrc/CMakeLists.txt
Outdated
Show resolved
Hide resolved
test/cpp_extensions/open_registration_extension/torch_openreg/torch_openreg/csrc/CMakeLists.txt
Outdated
Show resolved
Hide resolved
test/cpp_extensions/open_registration_extension/torch_openreg/torch_openreg/csrc/CMakeLists.txt
Outdated
Show resolved
Hide resolved
test/cpp_extensions/open_registration_extension/torch_openreg/cmake/TorchPythonTargets.cmake
Outdated
Show resolved
Hide resolved
test/cpp_extensions/open_registration_extension/torch_openreg/cmake/TorchPythonTargets.cmake
Outdated
Show resolved
Hide resolved
test/cpp_extensions/open_registration_extension/torch_openreg/csrc/CMakeLists.txt
Outdated
Show resolved
Hide resolved
.../cpp_extensions/open_registration_extension/torch_openreg/third_party/openreg/CMakeLists.txt
Outdated
Show resolved
Hide resolved
test/cpp_extensions/open_registration_extension/torch_openreg/torch_openreg/csrc/CMakeLists.txt
Outdated
Show resolved
Hide resolved
test/cpp_extensions/open_registration_extension/torch_openreg/torch_openreg/csrc/CMakeLists.txt
Outdated
Show resolved
Hide resolved
test/cpp_extensions/open_registration_extension/torch_openreg/torch_openreg/csrc/CMakeLists.txt
Outdated
Show resolved
Hide resolved
test/cpp_extensions/open_registration_extension/torch_openreg/torch_openreg/csrc/CMakeLists.txt
Outdated
Show resolved
Hide resolved
test/cpp_extensions/open_registration_extension/torch_openreg/setup.py
Outdated
Show resolved
Hide resolved
|
||
if(DEFINED PYTHON_INCLUDE_DIR) | ||
include_directories(${PYTHON_INCLUDE_DIR}) | ||
else() | ||
message(FATAL_ERROR "Cannot find Python directory") | ||
endif() | ||
|
||
include(${PROJECT_SOURCE_DIR}/cmake/TorchPythonTargets.cmake) |
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.
Hey @albanD, sorry to bother you.
PyTorch provides some CMake targets, such as torch_cpu_library
and torch_cuda_library
, but it doesn't provide a CMake target for torch_python
, which is very useful for out-of-tree backends, as they require some Python bindings. We currently wrap a CMake target for torch_python
in the openreg project, but it would be even better if Torch itself could provide one.
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.
Ho that is interesting.
I would have expected that our torch.utils.cpp_extension uses a cmake target to build these cpp extensions. But it doesn't?
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.
As far as I know, basic functions in torch.utils.cpp_extension
such as CPPExtension
use -ltorch_python
and specify library_dir
directly to build the extension instead of using cmake targets.
pytorch/torch/utils/cpp_extension.py
Lines 1204 to 1210 in 1ca8388
if not kwargs.get('py_limited_api', False): | |
# torch_python uses more than the python limited api | |
libraries.append('torch_python') | |
if IS_WINDOWS: | |
libraries.append("sleef") | |
kwargs['libraries'] = libraries |
In addition, requirements of torch.utils.cpp_extension
and integration of the new accelerator are slightly different; the core functions of the former are basically compiled into the extension library itself (such as _C.so), while the latter extension library itself is just an intermediate layer from Python to C++, and the core functions are all in the accelerator's own torch_binding.so. torch.uitls.cpp_extension
is more suitable for PyTorch users to extend some functions of torch (such as Pluggable Allocator), but it is not suitable for the latter. A highly integrated CMake object like torch_python_library
is what the latter needs.
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.
Ok!
Adding a new target sounds good then.
cc @swolchok can you give a quick look at the new cmake target? Is that the right style that we want going forward?
if(WIN32) | ||
set(TORCH_PYTHON_IMPORTED_LOCATION "${PYTORCH_INSTALL_DIR}/lib/torch_python.lib") | ||
set(TORCH_PYTHON_LINK_LIBS | ||
"${PYTORCH_INSTALL_DIR}/lib/c10.lib" | ||
"${PYTORCH_INSTALL_DIR}/lib/torch_cpu.lib" | ||
) | ||
set(TORCH_PYTHON_LIBRARY_LINK | ||
"\$<TARGET_FILE:torch_python>;\$<TARGET_PROPERTY:torch_python,INTERFACE_LINK_LIBRARIES>" | ||
) | ||
else() | ||
set(TORCH_PYTHON_IMPORTED_LOCATION "${PYTORCH_INSTALL_DIR}/lib/libtorch_python.so") | ||
set(TORCH_PYTHON_LINK_LIBS "c10;torch_cpu") | ||
set(TORCH_PYTHON_LIBRARY_LINK | ||
"-Wl,--no-as-needed,\"\$<TARGET_FILE:torch_python>\" -Wl,--as-needed;" | ||
"\$<TARGET_PROPERTY:torch_python,INTERFACE_LINK_LIBRARIES>" | ||
) | ||
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.
Why are all of these set by hand?
Also shouldn't we get these from the torch_python library below?
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.
Thank you for your advices.
- PyTorch does not provide
torch_python
ortorch_python_library
, which we intend to provide. - Besides manually specifying the
torch_python
library path, other properties can be unified and directly use CMake targets, such as c10 and torch_cpu provided by PyTorch, without manual settings. - We will also remove any unnecessary information.
@can-gaa-hou will do a update right now.
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.
@FFFrog, Thanks for pointing out some redundant lines of code and explaining what is going on here. I have removed some useless code. Please let me know if there are any other questions.
add_library(torch_python_library INTERFACE IMPORTED) | ||
|
||
set_target_properties(torch_python_library PROPERTIES | ||
INTERFACE_COMPILE_DEFINITIONS "\$<TARGET_PROPERTY:torch_python,INTERFACE_COMPILE_DEFINITIONS>" |
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.
Shouldn't all of these be standard properties?
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.
Yes, those definitions is not necessary for OpenReg
and will delete it.
add_library(torch_python SHARED IMPORTED) | ||
|
||
set_target_properties(torch_python PROPERTIES | ||
INTERFACE_COMPILE_DEFINITIONS "USE_DISTRIBUTED;USE_C10D_GLOO;USE_C10D_MPI;USE_RPC;USE_TENSORPIPE" |
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.
Why do we set these flags?
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.
ditto
if "CMAKE_BUILD_TYPE" not in os.environ: | ||
if check_env_flag("DEBUG"): | ||
os.environ["CMAKE_BUILD_TYPE"] = "Debug" | ||
elif check_env_flag("REL_WITH_DEB_INFO"): | ||
os.environ["CMAKE_BUILD_TYPE"] = "RelWithDebInfo" | ||
else: | ||
os.environ["CMAKE_BUILD_TYPE"] = "Release" |
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.
Why do we need this now?
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.
Got it, will dele it and keep DEBUG
as the default CMAKE_BUILD_TYPE
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.
Debug mode is quite complicated on Windows because it will load different DLLs at runtime and use python_d rather than python. Since OpenReg is still under development and there are not many lines of code, we can easily debug even if we build in Release Mode. To make this PR simple, I will support Debug mode in another PR.
import textwrap | ||
|
||
|
||
def _load_dll_libraries() -> None: |
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.
This is surprising that we have to do so much here? We don't usually do that for our cpp extensions.
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.
The dynamic library search mechanism on Windows differs from that on Linux/OSX, making it impossible to set RPATH
and LIBRARY_PATH
.
OpenReg is split into dynamic libraries: _C, torch_binding.so, torch_openreg.so, and openreg.so.
torch_openreg:
└── torch_openreg
├── lib
│ ├── torch_bindings.lib
│ ├── torch_openreg.lib
│ └── openreg.lib
└── _C.pyd
These libraries are not in the same directory as _C
, making it impossible for PyTorch to find these libraries on Windows.
Therefore, we referenced the relevant implementation in Torch:
Line 162 in e9d27aa
def _load_dll_libraries() -> None: |
The above method contains much information that OpenReg doesn't use. We should have adapted it, but we didn't, which is our problem and will fix it right now @can-gaa-hou
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.
You are right! Torch itself has already loaded a bunch of DLLs when we import torch on Windows; therefore, there is no need for us to load them twice when importing torch_openreg. I have fixed the code, and please let me know if there are any questions.
target_link_libraries(${LIBRARY_NAME} PRIVATE torch_python torch_openreg) | ||
target_link_libraries(${LIBRARY_NAME} PRIVATE torch_python_library torch_openreg) | ||
|
||
if(WIN32) |
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.
Why is this only windows?
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.
Same as below, Windows requires all symbols to be resolved at link time. We are using Python Dependency here to build our library; therefore, we need to explicitly find the Python library and link it to our target. I have refactored the code so that CMake will be executed for the same purpose on both Windows and macOS.
target_link_directories(${LIBRARY_NAME} PRIVATE ${PYTORCH_INSTALL_DIR}/lib) | ||
|
||
install(TARGETS ${LIBRARY_NAME} LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}) | ||
if(APPLE) | ||
set_target_properties(${LIBRARY_NAME} PROPERTIES LINK_FLAGS "-undefined dynamic_lookup") |
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.
Why do we need this one on macos?
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.
By default, Linux does not require all symbols to be resolvable when generating dynamic libraries. As long as the symbols can be resolved at runtime, it is fine. However, this is not the case on MAC. The effect of adding this -undefined dynamic_lookup
is the same as Linux. Unresolved Python symbols are ignored first and relocated at runtime.
PyTorch also uses the same approach, see this for more information.
@@ -284,6 +285,7 @@ def test_manual_seed(self): | |||
self.assertEqual(torch.openreg.initial_seed(), 2024) # type: ignore[misc] | |||
|
|||
# Autograd | |||
@skipIfWindows() |
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.
any details on what doesn't work on this?
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.
Lines 286 to 303 in 8ce81bc
# Autograd | |
def test_autograd_init(self): | |
# Make sure autograd is initialized | |
torch.ones(2, requires_grad=True, device="openreg").sum().backward() | |
pid = os.getpid() | |
task_path = f"/proc/{pid}/task" | |
all_threads = psutil.Process(pid).threads() | |
all_thread_names = set() | |
for t in all_threads: | |
with open(f"{task_path}/{t.id}/comm") as file: | |
thread_name = file.read().strip() | |
all_thread_names.add(thread_name) | |
for i in range(torch.accelerator.device_count()): | |
self.assertIn(f"pt_autograd_{i}", all_thread_names) |
In this testcase, we use "/proc/{pid}/task" to access a process, which works fine on Linux but not on Windows. If it is necessary to test autograd on Windows, I would like to support it :)
Hey @albanD, could you help to review this PR which have blocked other works related to |
As the title stated.
Changes:
Co-authored-by: @can-gaa-hou
Original PR:#159029