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 all commits
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
6 changes: 6 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,12 @@ include(CTest)
find_package(ROCM REQUIRED)
find_package(Threads REQUIRED)

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

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
7 changes: 6 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,12 @@ 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)
target_compile_definitions(driver PRIVATE MIGRAPHX_ENABLE_PYTHON)
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
31 changes: 14 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,24 @@
# THE SOFTWARE.
#####################################################################################

option(MIGRAPHX_ENABLE_PYTHON "Enable python bindings" ON)
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