Skip to content
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

[BUG] Redundant dependency acquisition #393

Closed
mtjrider opened this issue Jun 9, 2020 · 10 comments
Closed

[BUG] Redundant dependency acquisition #393

mtjrider opened this issue Jun 9, 2020 · 10 comments
Labels
bug Something isn't working CMake tech debt debt Internal clean up and improvements to reduce maintenance and technical debt in general

Comments

@mtjrider
Copy link
Contributor

mtjrider commented Jun 9, 2020

When building in CI/CD, spdlog needs to be installed twice.
Once during source configure/compile time where the C++ library is built.
Again in the Anaconda environment so that it is detected during Cythonization.
Ref.: rapidsai/integration#46

This redundancy is not specific to spdlog, but any library that is pulled in during cmake.
Opening this issue to track how this technical debt is addressed.

@mtjrider mtjrider added ? - Needs Triage Need team to review and classify bug Something isn't working labels Jun 9, 2020
@github-actions github-actions bot added this to Needs prioritizing in Bug Squashing Jun 9, 2020
@harrism harrism added CMake tech debt debt Internal clean up and improvements to reduce maintenance and technical debt in general and removed ? - Needs Triage Need team to review and classify labels Jun 11, 2020
@harrism harrism added this to Issue-Needs prioritizing in v0.15 Release via automation Jun 11, 2020
@harrism harrism moved this from Issue-Needs prioritizing to Issue-P1 in v0.15 Release Jun 11, 2020
@germasch
Copy link
Contributor

germasch commented Aug 4, 2020

I think I'm seeing a related build issue, ie., if I run ./build.sh, the C++ stuff builds fine, but then during the python build, I get

/usr/bin/gcc-7 -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -fPIC -fPIC -Irmm/_lib -I../include/rmm -I../include -I../build/include -I/opt/spack/opt/spack/linux-ubuntu18.04-haswell/gcc-7.5.0/python-3.7.6-uqr5escdjx6uoriv3xq6mnuvhc3ynomv/include -I/usr/local/cuda/include -I/opt/spack/opt/spack/linux-ubuntu18.04-haswell/gcc-7.5.0/python-3.7.6-uqr5escdjx6uoriv3xq6mnuvhc3ynomv/include/python3.7m -c rmm/_lib/memory_resource.cpp -o build/temp.linux-x86_64-3.7/rmm/_lib/memory_resource.o -std=c++14
In file included from rmm/_lib/memory_resource_wrappers.hpp:12:0,
                 from rmm/_lib/memory_resource.cpp:652:
../include/rmm/mr/device/logging_resource_adaptor.hpp:22:10: fatal error: spdlog/sinks/basic_file_sink.h: No such file or directory
 #include <spdlog/sinks/basic_file_sink.h>
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The reason being that while the C++ build clones and uses spdlog, that ends up not being found during the python build. I could hack around this by adding the relevant include directory, but I'm not sure this is the best solution.

This is partially caused by the fact that the python module build is separate from the cmake build, so the former doesn't directly know what the latter is doing. One possible solution is for the cmake build to properly install librmm.so, including dependencies, so that python setuptools can pick those up, but that may not be that straightforward. An alternative could be to build the python module via cmake directly.

In any case, it would certainly also generally desirable to have at least the option to build librmm.so against preinstalled packages rather than always relying on its own clones. In this header-mostly world, things probably don't usually go wrong, but generally it's not a good practice to compile librmm against, say spdlog-1.7.0 but then happen use it with an elsewhere installed spdlog-2.0.

In any case, I'm going to focus on the cmake/c++ build for now, while paying attention to not making this situation worse.

@harrism
Copy link
Member

harrism commented Aug 5, 2020

Hmmm, I would think others would have run into an issue with the python build by now if it was not specific to your setup @germasch ...

I use rapids-compose, so I'm somewhat shielded, but I believe all it does is run the python setup.py just as you would be doing.

@germasch
Copy link
Contributor

germasch commented Aug 5, 2020

I think the reason that others may not be seeing is that I didn't have spdlog installed on my system (a pretty bare bones Ubuntu 18.04 container). Once I installed spdlog in the container separately, the issue went away. That makes it sounds similar to the issue that started this thread, where you end up having two spdlog in the CI/CD, too.

@harrism harrism added this to Issue-Needs prioritizing in v0.16 Release via automation Aug 7, 2020
@harrism harrism removed this from Issue-P1 in v0.15 Release Aug 7, 2020
@harrism harrism added this to Issue-Needs prioritizing in v0.17 Release via automation Sep 28, 2020
@harrism harrism removed this from Issue-Needs prioritizing in v0.16 Release Sep 28, 2020
@harrism
Copy link
Member

harrism commented Oct 20, 2020

Is this an issue any longer after your recent changes, @germasch ?

@germasch
Copy link
Contributor

It's fixed for spdlog and googletest, where CPM will use an external install when available. There's still an issue with google benchmark, where the external install isn't recognized since they install a broken config. I'm not sure my PR google/benchmark#1047 is going anywhere...

And there's also a bunch of issues with finding an external Thrust -- most of the fixes have been accepted, but there's still some WIP.

@harrism
Copy link
Member

harrism commented Oct 20, 2020

So googletest and googlebenchmark versioning / packaging are handled differently? If so, they should consider working together. ;)

@germasch
Copy link
Contributor

I guess that's what googletest and googlemock did, but benchmark, not so much...

@harrism harrism removed this from Issue-Needs prioritizing in v0.17 Release Dec 3, 2020
@github-actions
Copy link

This issue has been marked rotten due to no recent activity in the past 90d. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.

@github-actions
Copy link

This issue has been marked stale due to no recent activity in the past 30d. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be marked rotten if there is no activity in the next 60d.

@rapidsai rapidsai deleted a comment from akagamy55 Feb 23, 2021
@rapidsai rapidsai deleted a comment from akagamy55 Feb 23, 2021
@rapidsai rapidsai deleted a comment from akagamy55 Feb 23, 2021
@rapidsai rapidsai deleted a comment from akagamy55 Feb 23, 2021
@rapidsai rapidsai deleted a comment from akagamy55 Feb 23, 2021
@rapidsai rapidsai deleted a comment from akagamy55 Feb 23, 2021
@kkraus14
Copy link
Contributor

This is solved now that we use CPMFindPackage for handling spdlog and other dependencies.

Bug Squashing automation moved this from Needs prioritizing to Closed Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CMake tech debt debt Internal clean up and improvements to reduce maintenance and technical debt in general
Projects
No open projects
Development

No branches or pull requests

4 participants