-
Notifications
You must be signed in to change notification settings - Fork 4
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
Rework how C++ wheels are handled in devcontainer builds #119
Comments
@trxcllnt pointed out that we could just reuse the |
I broadly support this. Here's the set of dependencies I see this being immediately relevant for:
Doing this would also allow us to drop some patches in DLFW, like the ones used to patch out the I agree that re-using the But what if...I can think of another implementation though. What if we added filtering support to Right now, cat "${requirement[@]}" "${pip_reqs_txts[@]}" \
| (grep -v '^#' || [ "$?" == "1" ]) \
| (grep -v -E '^$' || [ "$?" == "1" ]) \
| ( if test -n "${no_dedupe-}"; then cat -; else tr -s "[:blank:]" | LC_ALL=C sort -u; fi ) \
| (grep -v -P "^($(tr -d '[:blank:]' <<< "${pip_noinstall[@]/%/|}"))(=.*|>.*|<.*)?$" || [ "$?" == "1" ]) What if, instead, Like for creating an environment: matrix_selectors="cuda=${RAPIDS_CUDA_VERSION%.*};arch=$(arch);py=${RAPIDS_PY_VERSION};cuda_suffixed=true"
rapids-dependency-file-generator \
--output requirements \
--file-key "py_run_cugraph" \
--matrix "${matrix_selectors}" \
--exclude 'libucx-cu12' \
--exclude 'nvidia-nccl-cu12' \
| requirements.txt And for builds, it could be provided to pip wheel \
--no-build-isolation \
--no-deps \
--config-settings rapidsai.exclude-deps="libucx-cu12;nvidia-nccl-cu12" \
. We could bikeshed about exactly how to do that (wildcards? globs? regular expression? actually listing out literal distribution names?) and other implementation details. But in general.... I think there's something to this? It'd mean that the |
That seems like a good approach for this problem. I think we have enough data points illustrating what we need that it makes sense to search for ways to stop stuffing more complexity into dependencies.yaml and implement this sort of helpful feature in dfg/rbb. |
Currently C++ wheels are part of the devcontainer manifest and are therefore among the libraries built in those environments. However, in these environments the wheel build is designed to be a no-op because the library is already installed. The implementation involves 1) the CMake for the C++ exiting early if a
find_package
call finds that the library has already been built elsewhere, 2) the Python logic forload_library
allowing through various cases where no library is found under the assumption that the Python extension modules will have suitable RPATHs baked in to find libraries, and 3) various dependency shenanigans to ensure that we get the desired dependency lists in different cases (e.g. DLFW vs. pip devcontainers vs. conda devcontainers). These changes require us to jump through a number of hoops downstream because they force us to consider cases where a C++ wheel package may be installed but actually not contain any DSO. With the benefit of some experience, it's time that we reimagine how we handle these various cases.Rather than doing what we currently do, we can also satisfy all of these requirements by simply not installing the C++ wheels at all into environments where the system library is always preferred. The difficulty with accomplishing this so far in devcontainers and DLFW has been that this conflicts with the declared dependency lists, which state that e.g. pylibcudf depends on libcudf. Trying to remove the libcudf installation results in pip thinking the dependency is unsatisfied and going to install it. We can fix this by changing our dependencies.yaml files to ensure that when building with certain settings (config-settings for RBB that are passed to dfg's dependency generator) the C++ wheels are filtered out of the dependency list altogether. This solution should ensure that we never install C++ wheels except in environments where they are needed.
As a consequence of this, we will always have C++ wheels that contain DSOs (no more empty shells) and they can always default to loading the library from the wheel. They can also be consistently configured to allow loading system libraries (e.g. using an environment variable).
The text was updated successfully, but these errors were encountered: