-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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.
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
conda packaging for holoscan latest version (3.x) #28598
base: main
Are you sure you want to change the base?
Conversation
Hi! This is the staged-recipes linter and your PR looks excellent! 🚀 |
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( I do have some suggestions for making it better though... For recipes/holoscan/meta.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13403187480. Examine the logs at this URL for more detail. |
Is this the same as https://github.com/nvidia-holoscan/holoscan-sdk? If so, why re-package it when it is open source? |
Hi @isuruf, yes holoscan-sdk is the same thing. The main reason for repackaging being difficulty associated with building from source in terms of meeting build dependencies. In the recipe we have white-listed a few dependencies that are kernel modules (libxpmem.so, libv4l2.so), cuda drivers (libcuda.so, libnvidia-ml.so) for which there is no conda package and no docker image prebuilt w/ them AFAIK. Even with binary repackaging we have other hurdles to overcome before holoscan package can be released: conda packages that happen to be missing specific DSO files (libgdrapi.so missing in gds-tools, libonnxruntime.so.1.18.1 missing in onnxruntime 1.18.1, libtorchvision.so missing in torchvision), and then there are DSOs from a product that we don't have (yet)in conda-forge (TensorRT). For gds-tools, onnxruntime and torchvision I will see when we can get respective owners to provide missing DSO, and for TensorRT we'll have to find out a plan for it with respect to conda. |
… order to easily exclude host/runtime dependencies from getting into the package
Hi! This is the staged-recipes linter and your PR looks excellent but I have some suggestions. File-specific lints and/or hints:
|
Hi! This is the staged-recipes linter and your PR looks excellent! 🚀 |
Please ignore |
recipes/holoscan/build.sh
Outdated
cp -rv examples $PREFIX/ | ||
cp -rv lib $PREFIX/ | ||
cp -rv include $PREFIX/ | ||
cp -rv python $PREFIX/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are the pythons unpacked here instead of into site-package?
recipes/holoscan/meta.yaml
Outdated
@@ -0,0 +1,362 @@ | |||
{% set version = "3.0.0.1" %} | |||
{% set cuda_version = "12.6" %} # min version holoscan works with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the minimum version, then why do you implement CEC such that this package can be installed with CTK 12.0 packages?
recipes/holoscan/compile_examples.sh
Outdated
cmake -S $PREFIX/examples \ | ||
-DCMAKE_LIBRARY_PATH=${GCC_DIR} \ | ||
-DCMAKE_C_COMPILER=$CC \ | ||
-DCMAKE_CXX_COMPILER=$CXX \ | ||
-DCUDAToolkit_INCLUDE_DIRECTORIES="$PREFIX/include;$PREFIX/${targetsDir}/include" \ | ||
-DCMAKE_PREFIX_PATH=$CONDA_PREFIX \ | ||
-DCMAKE_CUDA_COMPILER=$PREFIX/bin/nvcc \ | ||
-DCUDAToolkit_ROOT=$PREFIX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try using $CMAKE_ARGS instead of manually setting all of these variables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this test takes a long time to build all the examples. Something shorter like
cmake -S $PREFIX/examples/hello_world/cpp $CMAKE_ARGS
might be more appropriate.
recipes/holoscan/meta.yaml
Outdated
run: | ||
- {{ pin_subpackage("libholoscan-dev", exact=True) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pin in both host and run when using exact pinnings, so that the packages are built in the correct order.
recipes/holoscan/meta.yaml
Outdated
- cuda-version {{ cuda_compiler_version }} # [cuda_compiler_version != "None"] | ||
run: | ||
- {{ pin_compatible('cuda-version', max_pin='x', min_pin='x') }} # [cuda_compiler_version != "None"] | ||
- {{ pin_subpackage("libholoscan" ~ soname, exact=True) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pin in both host and run when using exact pinnings.
recipes/holoscan/meta.yaml
Outdated
- python =3.10 | ||
- rdma-core |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we split the python artifacts into a separate output or are they part of the core functionality even when you are only using the C-API? Python is a large dependency for programs that don't use it.
- test -L $PREFIX/lib/libucm.so | ||
- test -L $PREFIX/lib/libucp.so | ||
- test -L $PREFIX/lib/libucs.so | ||
- test -L $PREFIX/lib/libucs_signal.so | ||
- test -L $PREFIX/lib/libuct.so | ||
- test -L $PREFIX/lib/libyaml-cpp.so |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these third-party libraries that are being revended?
- test -f $PREFIX/bin/video_validation.py | ||
- test -d $PREFIX/lib/cmake/holoscan | ||
- test -d $PREFIX/examples | ||
- test -d $PREFIX/include/3rdparty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be revending third party headers
recipes/holoscan/meta.yaml
Outdated
- test -L $PREFIX/lib/ucx/libuct_xpmem.so | ||
- test -L $PREFIX/lib/ucx/libucx_perftest_cuda.so | ||
|
||
- name: libholoscan-static |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The static library packages are meant to separate out optional static libraries when users have the option of linking to dynamic instead. For example, cudart vs cudart_static. In this case, this package only contains libholoviz_imgui.a and there is not an alternative shared library option? So a separate static library package is not needed.
check-glibc bin/* lib/* lib/ucx/* lib/gxf_extensions/* | ||
find python/ -name "*.so*" | xargs -I"{}" check-glibc "{}" | ||
|
||
cp -rv bin $PREFIX/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we unpacking a redist instead of building from source when the source is available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that we already discussed this above back in December 2024.
For gds-tools, onnxruntime and torchvision I will see when we can get respective owners to provide missing DSO, and for TensorRT we'll have to find out a plan for it with respect to conda.
Have you opened issues on the feedstocks for these packages asking for the relevant libraries?
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipes/holoscan/meta.yaml:
For recipes/holoscan/meta.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13471290947. Examine the logs at this URL for more detail. |
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( I do have some suggestions for making it better though... For recipes/holoscan/meta.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13514215908. Examine the logs at this URL for more detail. |
…python -> holoscan 2) libholoscan3 -> libholoscan
Checklist
url
) rather than a repo (e.g.git_url
) is used in your recipe (see here for more details).The following dependencies have been whitelisted to have the build going, but we need to provide them (or call them out as not supported) in conda. Only the checked items may be actionable.
Also, this package does not depend on MOFED because there is no MOFED available as conda package. Ref on dependencies: https://docs.nvidia.com/holoscan/sdk-user-guide/sdk_installation.html
Xref: #28623
This PR still very much WIP.