Fix conda custom operator build and self-tests - #6474
Conversation
4cda17c to
e16ab34
Compare
Conda installs DALI headers from libdali-devel in the active prefix, but get_include_dir() only returned the wheel-style package directory. Custom-operator builds therefore could not find the public DALI headers. Use the conda include directory when the active prefix contains DALI headers. The package-local include directory remains the fallback, preserving the behavior of wheel and non-conda installations. The conda self-test also searched PATH for test executables, although the conda package installs them beneath the nvidia.dali package. Search the local build output and installed package in the same order as TL0_self-test, report a missing executable clearly, and retain an absolute path for GoogleTest death tests. Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
e16ab34 to
bc678c1
Compare
 The PR appears safe to merge with no outstanding actionable findings.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
CI MESSAGE: [66908778]: BUILD STARTED |
Only use headers from CONDA_PREFIX when the imported nvidia.dali package is located under that same prefix. This prevents custom operators from compiling against conda headers while linking to a separately installed DALI wheel. Add focused sysconfig tests for both matching and mismatched installations. Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
|
@greptile review |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Compute the conda include path only where it is needed. This keeps the matching-installation guard while avoiding CodeQL's unused-local diagnostic. Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
|
@greptile review |
|
CI MESSAGE: [66916279]: BUILD STARTED |
|
CI MESSAGE: [66916279]: BUILD FAILED |
|
CI MESSAGE: [66959790]: BUILD STARTED |
|
CI MESSAGE: [66959790]: BUILD PASSED |
| # https://google.github.io/googletest/advanced.html#death-test-styles which runs tests in | ||
| # a separate process don't use PATH to discover the file location and fails | ||
| FULLPATH="" | ||
| for DIRNAME in \ |
There was a problem hiding this comment.
[Moderate] I think this search list is missing the one location where the conda package actually puts the test binaries, so it may not find them at all.
conda/dali_native_libs/recipe/build.sh ends with a loop that copies every built artifact out of the wheel-style layout, and for the test binaries the destination is the prefix bin dir:
if [[ "$FILE" == *".bin"* ]]; then
cp $FILE $PREFIX/bin/;
fiThat is exactly why the old which $BINNAME worked -- enable_conda activates the env, so $CONDA_PREFIX/bin is on PATH. The binaries are not package-private in the conda layout; they live in $CONDA_PREFIX/bin/<name>.bin, not in <package>/test/.
The conda bindings recipe builds only dali_python python_function_plugin copy_post_build_target dali_python_generate_stubs install_headers, so no test targets are produced in build_bindings, and the pip-installed conda package should have no test/ directory at all. That leaves candidate 2 empty and makes the whole thing depend on candidate 1.
Two consequences:
- If
../../build/dali/python/nvidia/dalidoes not exist in the conda test image, every binary hitsERROR: $BINNAME not foundand the job fails. - If it does exist, the conda self-test silently runs the non-conda build's binaries inside a conda env -- which defeats the point of this test.
For (2), note the ordering matters differently here than in TL0: for the conda job the installed conda package should win over a stray local build tree, not the other way round.
Suggest searching full paths rather than parent dirs, so $CONDA_PREFIX/bin (flat) and <dir>/test (nested) can both be expressed, and putting the conda location first:
for CANDIDATE in \
"${CONDA_PREFIX:+$CONDA_PREFIX/bin/$BINNAME}" \
"$(python -c '...')/test/$BINNAME" \
"../../build/dali/python/nvidia/dali/test/$BINNAME"
do
if [ -x "$CANDIDATE" ]; then FULLPATH=$(readlink -f "$CANDIDATE"); break; fi
doneIf I have the conda layout wrong and the binaries really are under <package>/test there, please say so -- but then it would be worth explaining in the description what was actually broken, since which should have found them either way.
| exit 1 | ||
| fi | ||
|
|
||
| # Invoke the test binary with an absolute path so |
There was a problem hiding this comment.
[Minor] The comment says "absolute path", but neither candidate produces one -- ../../build/dali/python/nvidia/dali/test/... is relative, and the death-test rationale it is justifying is specifically about not depending on the process's notion of where the executable is. The python -c candidate happens to be absolute; the first one is not.
Either wrap the result in readlink -f/realpath when assigning FULLPATH, which makes the comment true for both candidates, or reword it to "an explicit path rather than a bare name resolved via PATH", which is what the code actually guarantees.
| # PackedBFrames test is disabled because it doesn't work with the conda upstream build | ||
| # of FFMpeg | ||
| $(which $BINNAME) --gtest_filter="*:-*PackedBFrames*" | ||
| DALI_USE_EXEC2=0 "$FULLPATH" --gtest_filter="*:-*PackedBFrames*" |
There was a problem hiding this comment.
[Minor] DALI_USE_EXEC2=0 is a behavior change that is not mentioned anywhere in the PR description or the commit message -- this job previously ran with the default executor. I assume it was carried over from qa/TL0_self-test/test.sh (which pins 0, with TL0_self-test-exec2 covering 1), and pinning is reasonable, but for the conda job it means dropping exec2 coverage with no exec2 counterpart to pick it up.
Worth an explicit line in the description, and worth confirming it is intentional rather than an artifact of copying the TL0 body.
| if package_is_in_conda_prefix and os.path.isdir(os.path.join(conda_prefix, "include", "dali")): | ||
| return os.path.join(conda_prefix, "include") | ||
|
|
||
| return package_include_dir |
There was a problem hiding this comment.
[Minor] get_include_dir is now conda-aware but get_lib_dir (just below) still returns the package directory unconditionally, and get_link_flags builds -L<package dir> -ldali from it.
If the premise of this PR is that a conda install does not ship package-private headers, it is worth stating why the same is not true for libdali.so -- a custom operator build that now compiles against $CONDA_PREFIX/include will still link against <package>/libdali.so. I believe the bindings recipe does place the prebuilt libs in the package dir (PREBUILD_DALI_LIBS=ON), so this is probably fine in practice, but the asymmetry is the kind of thing that bites later. A one-line comment recording that libs stay package-local on purpose would be enough.
|
|
||
| with mock.patch.dict(os.environ, {"CONDA_PREFIX": conda_prefix}): | ||
| with mock.patch.object(dali, "__file__", package_file): | ||
| assert dali_sysconfig.get_include_dir() == os.path.join(package_dir, "include") |
There was a problem hiding this comment.
[Minor] The two new tests cover the two package_is_in_conda_prefix outcomes, but not the second half of the condition: package is inside $CONDA_PREFIX, yet $CONDA_PREFIX/include/dali does not exist. That branch is the one protecting a conda env where libdali-devel is not installed, and it is the case most likely to regress if someone later simplifies the condition.
It is a three-line addition to the existing pattern -- same setup as test_sysconfig_uses_conda_headers_for_conda_package but skipping the os.makedirs, asserting the package-local path is returned.
5 participants
Fix conda custom operator build and self-tests.
Conda installs DALI headers from
libdali-develin the active prefix, while DALI sysconfig previously exposed only the wheel-style package include directory. The conda self-test similarly assumed its package-private test executables were onPATH.Category:
Bug fix
Description:
Use
$CONDA_PREFIX/includeonly when the imported DALI package belongs to that prefix, retaining the package-local directory for wheels and other installations. Make the conda self-test search local build output and the installed DALI package as TL0 does, while retaining absolute executable paths for GoogleTest death tests.Additional information:
Affected modules and functionalities:
nvidia.dali.sysconfiginclude flags for custom-operator builds.Key points relevant for the review:
$CONDA_PREFIX/include/daliexists and the imported package is under$CONDA_PREFIX.Tests:
test_plugin_manager.py: conda sysconfig header-path selection.qa/TL1_self-test_conda/test.shdiscovery behavior.Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: N/A