Fix build-time race condition for NPU VM target#36448
Conversation
openvino_npu_vm_utils may be compiled before Level Zero headers are copied into the build directory, causing a missing-header error on parallel builds. Add an explicit dependency on prepare_ze_headers to ensure correct build ordering. Signed-off-by: Gleb Gladilov <8751635+ggladilov@users.noreply.github.com>
| # headers are copied into the build directory by prepare_ze_headers; without | ||
| # this explicit dependency a parallel build may compile ${TARGET_NAME} before | ||
| # the headers are copied, causing a missing-header error | ||
| add_dependencies(${TARGET_NAME} prepare_ze_headers) |
There was a problem hiding this comment.
More idiomatic solution in CMake would be to make build-dependency explicit via target_link_libraries. The idea is that if one targets needs to include a header file from another target, likely it also needs to link against implementation.
However, I decided to go with more conservative option, since I don't have the design background for NPU VM.
There was a problem hiding this comment.
I think it should be safe to use target_link_libraries in src/plugins/intel_npu/src/vm/runtime/CMakeLists.txt, so that the npu_vm_runtime_api target handles the necessary headers by default. Something like this should work:
target_link_libraries(${TARGET_NAME} INTERFACE openvino::zero_loader openvino::level-zero-extopenvino::zero_loader-> provideslevel_zero/ze_api.hopenvino::level-zero-extprovidesze_graph_ext.h
There was a problem hiding this comment.
What you propose seems to create build-edge between npu_vm_runtime_api and prepare_ze_headers. But how does it help openvino_npu_vm_utils? I don't see it depending on npu_vm_runtime_api
There was a problem hiding this comment.
openvino_npu_vm_utils's implementation transitively includes intel_npu/runtime/npu_vm_runtime.hpp, so I would expect a dependency to npu_vm_runtime_api to exist as well. There might be a gap here as well.
Recently, the npu_vm_runtime.hpp header has been moved to the VM library (found in the compiler repo) which implements the header methods. I expect that with an upcoming compiler integration, this header (together with the npu_vm_runtime_api target) could be removed from OV and the VM library could be used as a dependency. So it's likely that this dependency issue will be solved then. I am not sure however when this change will be done, so it should be sufficient for the time being to leave the direct dependency on prepare_ze_headers. What do you think @PatrikStepan?
There was a problem hiding this comment.
openvino_npu_vm_utils's implementation transitively includes intel_npu/runtime/npu_vm_runtime.hpp, so I would expect a dependency to npu_vm_runtime_api to exist as well.
I don't think there's a dependency of openvino_npu_vm_utils on npu_vm_runtime_api, the same way there is no dependency of openvino_npu_vm_utils on prepare_ze_headers - root cause of the issue.
Transitive include is irrelevant, C++ code doesn't impact CMake's build dependency tree (#include doesn't guarantee the file is available). The same is true when one target does target_include_directories and/or uses generator-expression (e.g. with TARGET_PROPERTY) - all it does is extends build flags with extra -I or queries a target at configuration stage.
None of the above impact generation stage / build.
This is why the fix is to add either add_dependency or target_link_libraries - these in contrast with the above instruct CMake that one target has to be executed or generated before another.
Also, this is why it's one of the CMake's guidelines to use target_link_libraries to connect one target with another. Basically the rule is to treat targets as in Object-Oriented Programming, where instead of accessing properties of a dependency directly via target_include_directories, target_compile_options or target_compile_definitions, one should:
- Take care what properties (include directories, compiler definitions, etc.) are public vs private
- Use
target_link_librariesto both/either extend link command and connect one target with another (automatically propagates all interface properties and creates build-time edge)
There was a problem hiding this comment.
I expect that with an upcoming compiler integration, this header (together with the npu_vm_runtime_api target) could be removed from OV and the VM library could be used as a dependency.
@hrotuna the remove of header on OV side will not happen. We need header to find the symbol of VM lib. Just ov and vpux to use their own version.
For this PR, we have a refactor PR #36099 in progress. Still nice to have this fix, we may keep or just move the header to ov_npu_vm_utils later since no need to keep an interface any more. FYI
| # headers are copied into the build directory by prepare_ze_headers; without | ||
| # this explicit dependency a parallel build may compile ${TARGET_NAME} before | ||
| # the headers are copied, causing a missing-header error | ||
| add_dependencies(${TARGET_NAME} prepare_ze_headers) |
There was a problem hiding this comment.
I expect that with an upcoming compiler integration, this header (together with the npu_vm_runtime_api target) could be removed from OV and the VM library could be used as a dependency.
@hrotuna the remove of header on OV side will not happen. We need header to find the symbol of VM lib. Just ov and vpux to use their own version.
For this PR, we have a refactor PR #36099 in progress. Still nice to have this fix, we may keep or just move the header to ov_npu_vm_utils later since no need to keep an interface any more. FYI
|
build_jenkins |
655b967
### Details: openvino_npu_vm_utils may be compiled before Level Zero headers are copied into the build directory, causing a missing-header error on parallel builds. Add an explicit dependency on prepare_ze_headers to ensure correct build ordering. ### Tickets: - https://jira.devtools.intel.com/browse/CVS-188745 ### AI Assistance: - *AI assistance used: yes* - AI was asked to find the reason for build issue (missing header during VPUx build), check if there're other related issues with the same CMake target and propose a fix. Also AI was asked to propose a reproducer to make issue visible locally on Ubuntu (originally build failed only on Windows pre-commit of VPUx). I manually tested the build locally on Ubuntu, checked with reproducer (manually remove copied headers and try again) that issue now disappears and tested in Windows VPUx pre-commit, where issue were originally reproduced to see if it was solved Signed-off-by: Gleb Gladilov <8751635+ggladilov@users.noreply.github.com>
Details:
openvino_npu_vm_utils may be compiled before Level Zero headers are copied into the build directory, causing a missing-header error on parallel builds. Add an explicit dependency on prepare_ze_headers to ensure correct build ordering.
Tickets:
AI Assistance: