make pybind compilation optional #2280
Conversation
CMakeLists.txt
Outdated
| @@ -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) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We would still want to have an option to enable it, otherwise we wont be able to test this without modifying the cmake.
src/py/CMakeLists.txt
Outdated
| @@ -22,27 +22,25 @@ | |||
| # THE SOFTWARE. | |||
| ##################################################################################### | |||
|
|
|||
| option(MIGRAPHX_ENABLE_PYTHON "Enable python bindings" ON) | |||
| add_compile_definitions($<$<COMPILE_LANGUAGE:CXX,C>:MIGRAPHX_ENABLE_PYTHON>) | |||
There was a problem hiding this comment.
This should only be added to the driver: target_compile_definitions(driver MIGRAPHX_ENABLE_PYTHON=1)
| 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() |
There was a problem hiding this comment.
Can you preserve the formatting? Its hard to tell what is changed here.
There was a problem hiding this comment.
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.
Codecov Report
@@ Coverage Diff @@
## develop #2280 +/- ##
========================================
Coverage 91.45% 91.45%
========================================
Files 433 433
Lines 16177 16177
========================================
Hits 14795 14795
Misses 1382 1382 |
This build is OK for merge ✅ |
🔴bert_base_cased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output🔴distilgpt2_fp16: FAILED: MIGraphX is not within tolerance - check verbose output |
UAI does not depend on PyBind, and there are no plans to port that part to Windows (at least at the moment).