Generate API sources in the build directory#4961
Conversation
There was a problem hiding this comment.
Pull request overview
This PR changes the C API build to generate migraphx.h and api.cpp into the build tree and compile/install from those generated outputs, rather than relying on the checked-in src/api copies.
Changes:
- Extend
tools/generate.pywith an--api-onlymode that writes C API outputs to caller-provided paths and can optionally skipclang-format. - Update
src/api/CMakeLists.txtto add a custom command/target that generates the C API into${CMAKE_CURRENT_BINARY_DIR}and buildsmigraphx_cfrom the generatedapi.cpp. - Install C API headers from the build-generated include directory rather than the source directory.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/generate.py | Adds --api-only generation path and makes formatting optional for build-only generation. |
| src/api/CMakeLists.txt | Generates C API sources/headers into the build dir and builds/installs migraphx_c from those outputs. |
|
|
||
| add_custom_command( | ||
| OUTPUT ${MIGRAPHX_API_GEN_HEADER} ${MIGRAPHX_API_GEN_SOURCE} | ||
| COMMAND ${Python_EXECUTABLE} ${MIGRAPHX_API_TOOLS_DIR}/generate.py |
| # tools/api. `make generate` still writes them into the source tree so the | ||
| # output can be reviewed, but migraphx_c is built from a freshly generated copy | ||
| # placed in the build directory instead of the checked-in src/api copies. | ||
| find_package(Python 3 COMPONENTS Interpreter REQUIRED) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #4961 +/- ##
===========================================
+ Coverage 92.71% 93.01% +0.30%
===========================================
Files 589 592 +3
Lines 31160 31217 +57
===========================================
+ Hits 28888 29035 +147
+ Misses 2272 2182 -90 🚀 New features to boost your workflow:
|
Regressions detected 🔴 * No develop baseline was found for this PR's branch point; compared against the latest available develop run instead. |
|
|
Is the PR just an API generator? It should be merged with the one I submitted for making TF and ONNX optional. The purpose of #3972 is to make TF and ONNX optional in the MIGraphX C API (migraphx_c.dll). Alternatively, to avoid excluding anything, we could move the MIGraphX TF C API to |
Yes this just refactors it generate the API in the build directory while preserving the generation in source. Then #3972 can build on top of it to make the modules optional.
That does seem like a better approach, but it would be a breaking change since the user will need to include a different header. If most people use the migraphx.hpp header we can include it(with a Also, we would create |
Motivation
This is to help enable conditional compilation of our API such as disabling TF or ONNX frontends.
Technical Details
Changelog Category
Add a
CHANGELOG.mdentry for any option other thanNot Applicable