Skip to content
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.

Sign up for GitHub

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

make pybind compilation optional #2280

Merged
merged 6 commits into from
Oct 11, 2023
Merged

make pybind compilation optional #2280

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9c8e8ac
make pybind compilation optional
apwojcik Oct 3, 2023
a8968d7
incorporate review feedback
apwojcik Oct 4, 2023
dd78c66
Merge branch 'develop' into make_pybind_optional
apwojcik Oct 4, 2023
1a38cd6
option: incorporate review feedback
apwojcik Oct 7, 2023
f1bbd40
Merge branch 'develop' into make_pybind_optional
apwojcik Oct 7, 2023
470c055
Merge branch 'develop' into make_pybind_optional
causten Oct 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ include(CTest)
find_package(ROCM REQUIRED)
find_package(Threads REQUIRED)

include(CMakeDependentOption)
cmake_dependent_option(MIGRAPHX_ENABLE_PYTHON "Enable python bindings" ON "WIN32" OFF)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we write this as:

if(WIN32)
option(MIGRAPHX_ENABLE_PYTHON "Enable python bindings" OFF)
else()
option(MIGRAPHX_ENABLE_PYTHON "Enable python bindings" ON)
endif()

Thats the way do other options.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The library migraphx_py is not entirely compiling on Windows. I want to avoid issues reported about that fact by not exposing the option on Windows. That may change in the future as soon as complete support is introduced. However, at the moment, it is not our priority. Please let me know if you still want me to change it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would still want to have an option to enable it, otherwise we wont be able to test this without modifying the cmake.


find_path(HALF_INCLUDE_DIR half.hpp PATH_SUFFIXES half)
if (NOT HALF_INCLUDE_DIR)
message(FATAL_ERROR "Could not find half.hpp - Please check that the install path of half.hpp has been added to CMAKE_PREFIX_PATH")
Expand Down
2 changes: 2 additions & 0 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,9 @@ add_subdirectory(driver)
add_subdirectory(onnx)
add_subdirectory(tf)

if(MIGRAPHX_ENABLE_PYTHON)
add_subdirectory(py)
endif()
add_subdirectory(targets/ref)
target_link_libraries(migraphx_all_targets INTERFACE migraphx_ref)
if(MIGRAPHX_ENABLE_CPU)
Expand Down
6 changes: 5 additions & 1 deletion src/driver/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,11 @@ rocm_clang_tidy_check(driver)
file(STRINGS "${CMAKE_SOURCE_DIR}/test/onnx/.onnxrt-commit" String_output)
target_compile_definitions(driver PUBLIC MIGRAPHX_ORT_SHA1="${String_output}")

target_link_libraries(driver migraphx_all_targets migraphx_onnx migraphx_tf migraphx_py)
target_link_libraries(driver migraphx_all_targets migraphx_onnx migraphx_tf)

if(MIGRAPHX_ENABLE_PYTHON)
target_link_libraries(driver migraphx_py)
endif()

rocm_install_targets(
TARGETS driver
Expand Down
4 changes: 4 additions & 0 deletions src/driver/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@

#include <migraphx/tf.hpp>
#include <migraphx/onnx.hpp>
#ifdef MIGRAPHX_ENABLE_PYTHON
#include <migraphx/py.hpp>
#endif
#include <migraphx/stringutils.hpp>
#include <migraphx/convert_to_json.hpp>
#include <migraphx/load_save.hpp>
Expand Down Expand Up @@ -281,10 +283,12 @@ struct loader
options.format = "json";
p = migraphx::load(file, options);
}
#ifdef MIGRAPHX_ENABLE_PYTHON
else if(file_type == "py")
{
p = migraphx::load_py(file);
}
#endif
else if(file_type == "migraphx")
{
p = migraphx::load(file);
Expand Down
32 changes: 15 additions & 17 deletions src/py/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,27 +22,25 @@
# THE SOFTWARE.
#####################################################################################

option(MIGRAPHX_ENABLE_PYTHON "Enable python bindings" ON)
add_compile_definitions($<$<COMPILE_LANGUAGE:CXX,C>:MIGRAPHX_ENABLE_PYTHON>)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should only be added to the driver: target_compile_definitions(driver MIGRAPHX_ENABLE_PYTHON=1)

add_library(migraphx_py py_loader.cpp)
migraphx_generate_export_header(migraphx_py)
target_include_directories(migraphx_py PRIVATE include)
target_link_libraries(migraphx_py PUBLIC migraphx)
rocm_install_targets(TARGETS migraphx_py INCLUDE include)
if(MIGRAPHX_ENABLE_PYTHON)
include(PythonModules)

include(PythonModules)

foreach(PYTHON_VERSION ${PYTHON_VERSIONS})
py_add_module(migraphx_pybind_${PYTHON_VERSION} migraphx_py.cpp PYTHON_VERSION ${PYTHON_VERSION} PYTHON_MODULE migraphx)
target_link_libraries(migraphx_pybind_${PYTHON_VERSION} PRIVATE migraphx migraphx_tf migraphx_onnx migraphx_all_targets)
rocm_install_targets(TARGETS migraphx_pybind_${PYTHON_VERSION})
add_dependencies(migraphx_py migraphx_pybind_${PYTHON_VERSION})

add_library(migraphx_py_${PYTHON_VERSION} py.cpp)
target_include_directories(migraphx_py_${PYTHON_VERSION} PRIVATE include)
target_link_libraries(migraphx_py_${PYTHON_VERSION} PUBLIC migraphx)
target_link_libraries(migraphx_py_${PYTHON_VERSION} PRIVATE pybind11::pybind11 python${PYTHON_VERSION}::runtime)
rocm_install_targets(TARGETS migraphx_py_${PYTHON_VERSION})
add_dependencies(migraphx_py migraphx_py_${PYTHON_VERSION})
endforeach()
endif()
foreach(PYTHON_VERSION ${PYTHON_VERSIONS})
py_add_module(migraphx_pybind_${PYTHON_VERSION} migraphx_py.cpp PYTHON_VERSION ${PYTHON_VERSION} PYTHON_MODULE migraphx)
target_link_libraries(migraphx_pybind_${PYTHON_VERSION} PRIVATE migraphx migraphx_tf migraphx_onnx migraphx_all_targets)
rocm_install_targets(TARGETS migraphx_pybind_${PYTHON_VERSION})
add_dependencies(migraphx_py migraphx_pybind_${PYTHON_VERSION})

add_library(migraphx_py_${PYTHON_VERSION} py.cpp)
target_include_directories(migraphx_py_${PYTHON_VERSION} PRIVATE include)
target_link_libraries(migraphx_py_${PYTHON_VERSION} PUBLIC migraphx)
target_link_libraries(migraphx_py_${PYTHON_VERSION} PRIVATE pybind11::pybind11 python${PYTHON_VERSION}::runtime)
rocm_install_targets(TARGETS migraphx_py_${PYTHON_VERSION})
add_dependencies(migraphx_py migraphx_py_${PYTHON_VERSION})
endforeach()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you preserve the formatting? Its hard to tell what is changed here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved if(MIGRAPHX_ENABLE_PYTHON) ... endif() from here to src/CMakeLists.txt. I wanted the file preamble to be guarded by that check, too (because of configuration issues on Windows). That is also why the indentation changed. Additionally, I moved option(MIGRAPHX_ENABLE_PYTHON...) to master CMakeLists.txt so test/CMakeLists.txt and test/py/CMakeLists.txt can see it and interpret it correctly - before the test/py/CMakeLists.txt could not see migraphx_pybind_3.8 target when Python was enabled.

Loading