Skip to content
This repository has been archived by the owner on Jun 16, 2022. It is now read-only.

libcugraph leaks the RMM dependency to downstream #10

Closed
leofang opened this issue Apr 23, 2021 · 10 comments · Fixed by #12
Closed

libcugraph leaks the RMM dependency to downstream #10

leofang opened this issue Apr 23, 2021 · 10 comments · Fixed by #12

Comments

@leofang
Copy link
Member

leofang commented Apr 23, 2021

tl;dr: I don't think the current handling of downloading RMM as part of source code

# libcugraph depends on the RMM header-only library of the same version.
- url: https://github.com/rapidsai/rmm/archive/refs/tags/v{{ version }}.tar.gz
sha256: eddd5b35b016d665eb4e8ea2dd31497d913b3e1eb831124e4cb27cba747267a0
folder: rmm

is appropriate. RMM must be packaged as a Conda-Forge package so that libcugraph can depend on it, and downstream packages like CuPy can just depend on libcugraph without worrying about RMM.

From conda-forge/cupy-feedstock#123 (comment):

    building 'cupy_backends.cuda.libs.cugraph' extension
    /home/conda/feedstock_root/build_artifacts/cupy_1619157077988/_build_env/bin/x86_64-conda-linux-gnu-cc -DNDEBUG -fwrapv -O2 -Wall -Wstrict-prototypes -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -ffunction-sections -pipe -isystem /home/conda/feedstock_root/build_artifacts/cupy_1619157077988/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeh/include -fdebug-prefix-map=/home/conda/feedstock_root/build_artifacts/cupy_1619157077988/work=/usr/local/src/conda/cupy-9.0.0 -fdebug-prefix-map=/home/conda/feedstock_root/build_artifacts/cupy_1619157077988/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeh=/usr/local/src/conda-prefix -I/usr/local/cuda/include -DNDEBUG -D_FORTIFY_SOURCE=2 -O2 -isystem /home/conda/feedstock_root/build_artifacts/cupy_1619157077988/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeh/include -I/usr/local/cuda/include -fPIC -D_FORCE_INLINES=1 -DCUPY_CUB_VERSION_CODE=100910 -DCUPY_JITIFY_VERSION_CODE=-1 -I/usr/local/cuda/include/cub -I/tmp/pip-req-build-f19x1qnn/install/../cupy/_core/include -I/usr/local/cuda/include -I/home/conda/feedstock_root/build_artifacts/cupy_1619157077988/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeh/include -I/home/conda/feedstock_root/build_artifacts/cupy_1619157077988/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeh/include/cugraph -I/home/conda/feedstock_root/build_artifacts/cupy_1619157077988/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeh/include/python3.6m -c cupy_backends/cuda/libs/cugraph.cpp -o build/temp.linux-x86_64-3.6/cupy_backends/cuda/libs/cugraph.o
    cc1plus: warning: command line option '-Wstrict-prototypes' is valid for C/ObjC but not for C++
    In file included from /home/conda/feedstock_root/build_artifacts/cupy_1619157077988/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeh/include/cugraph/algorithms.hpp:18:0,
                     from cupy_backends/cuda/libs/../../cupy_cugraph.h:12,
                     from cupy_backends/cuda/libs/cugraph.cpp:677:
    /home/conda/feedstock_root/build_artifacts/cupy_1619157077988/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeh/include/cugraph/dendrogram.hpp:18:10: fatal error: rmm/device_uvector.hpp: No such file or directory
     #include <rmm/device_uvector.hpp>
              ^~~~~~~~~~~~~~~~~~~~~~~~
    compilation terminated.

https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=307796&view=logs&j=7e9c1385-f003-5191-6e56-de8b58facc56&t=e14c8b26-d4de-545b-9aea-e32da246fb0e&l=4922

It shows RMM headers must be present when building applications depending on libcugraph. But, given that RMM must be pinned at the same version as libcugraph, it is impossible for downstream packages to handle this pinning, and instead libcugraph should do this via pin_compatible, which in turn requires RMM to be available as a standalone package.

cc: @kkraus14 @jakirkham @rlratzel

@h-vetinari
Copy link
Member

Also cc @isuruf

@rlratzel
Copy link

rlratzel commented Apr 23, 2021

Can we possibly also include the rmm headers in the libcugraph package via the build script until we fix the problem in libcugraph's headers?

@leofang
Copy link
Member Author

leofang commented Apr 23, 2021

Even as a workaround I don't think it's ok. If there are more than one package bundling the RMM headers, when installing them at the same time they would overwrite each other's bundle in $CONDA_PREFIX/include/ and cause chaos, not to mention other subtle potential symbol conflicts.

I think the right way is to follow Thrust and CUB, which are also header-only libraries, and make RMM a standalone package. Maybe @jakirkham @kkraus14 or other members from @conda-forge/core can comment and offer a better band-aid fix.

@rlratzel
Copy link

Even as a workaround I don't think it's ok. If there are more than one package bundling the RMM headers, when installing them at the same time they would overwrite each other's bundle in $CONDA_PREFIX/include/ and cause chaos, not to mention other subtle potential symbol conflicts.

I think the right way is to follow Thrust and CUB, which are also header-only libraries, and make RMM a standalone package.

Thanks @leofang , that makes sense and I agree. We may be able to address this on the cugraph side, but unfortunately not for 0.19. I can start working on the RMM recipe for conda-forge if other members don't have a better idea.

@isuruf
Copy link
Member

isuruf commented Apr 23, 2021

Having a separate package is the best idea.

@kkraus14
Copy link

We're going to add a librmm package to resolve this

@rlratzel
Copy link

I've submitted a PR for librmm: conda-forge/staged-recipes#14749

@h-vetinari
Copy link
Member

https://github.com/conda-forge/librmm-feedstock has been created and the packages made it through the CDN. Should be ready to start depending on librmm here. 🙃

@leofang
Copy link
Member Author

leofang commented Apr 28, 2021

Maybe (?) blocked by conda-forge/librmm-feedstock#2.

@jakirkham
Copy link
Member

jakirkham commented Apr 28, 2021

Looks like that should be fixed. So should be unblocked now

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants