[REREVIEW] Build infrastructure to use RAFT #2125

dantegd · 2020-04-23T01:35:25Z

No description provided.

GPUtester · 2020-04-23T01:35:56Z

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

JohnZed

Looks great! Only tiny comments

.gitignore

python/cuml/nccl/nccl.pyx

cpp/cmake/Dependencies.cmake

python/setup.py

python/setuputils.py

JohnZed · 2020-04-24T00:10:11Z

python/setuputils.py

+
+        print("-- Third party repositories have not been found so they " +
+              "will be cloned. To avoid this set the environment " +
+              "variable CUML_BUILD_PATH, containing the relative " +


Why does it have to be relative? Abs paths could be clearer

Removed all relative imports, I think everything being absolute is just better for consistency, what do you think?

python/setuputils.py

cpp/cmake/Dependencies.cmake

python/cuml/__init__.py

teju85 · 2020-04-26T05:41:06Z

python/setuputils.py

+    return repo_path, repo_cloned
+
+
+def get_submodule_dependency(repo,


In general, I'm not too comfortable with parsing cmake files inside python setup.py. You did mention the reason for doing this cloning in python sometime ago, @dantegd and I seem to have forgotten it now! Can you please (re)help me remember the reason for doing this, please?

It is for cloning RAFT if needed by Python (or any other repo), and having only one central place where RAFT is pinned. Otherwise we would need to have 2 places where the pinned commit of RAFT is defined, introducing potential conflicts.

Typically, RAFT will only be cloned by cmake and python uses that RAFT clone, or RAFT will be located in the location pointed by RAFT_PATH and both cmake and python use it from there. But if neither is true and a developer is only building the Python package, then python clones it itself (this was the same situation with treelite and other things that are/were needed by the python package)

To solve the need to parse cmake from setup.py and decouple things, we could create a config file that both cmake and setup.py read and is clearer/easier for devs to modify, what do you think?

Ideally, none of the treelite/faiss/spdlog dependencies should have been exposed to the users who'd only want to build python code (but have libcuml++.so and cpp/include headers installed already)! So, clearly, this seems to be an issue with our C++ layer. I have filed issue #2138 to address this one.

teju85 · 2020-04-26T05:43:57Z

cpp/cmake/Dependencies.cmake

+
+# Only cloned if RAFT_PATH env variable is not defined
+
+if(DEFINED ENV{RAFT_PATH})


@dantegd @JohnZed @cjnolet For now the Dependencies.cmake changes are fine since we have agreed to keep raft as a header-only library. What if that changes in future and we might have to build binaries? Then this if-else block would become difficult to support, no?

If we need to build binaries, then we will need to reopen design discussions and overall will have big implications. It is agreed right now with the cuGraph team to keep it header only, if that changes then this will need to change, and might introduce the need for a conda package distribution and other matters

But thinking about it, I don’t think that, if a binary is needed, it would need a massive change. Mainly having to modify the CMakeLists in RAFT, then that gets included by libcugraph/libcuml and then (presumably) statically linking the libraft generated, no?

Good point. How about doing something similar to what you've been doing to share RAFT's python code? In other words, we expose a RAFT_SRC_FILES cmake variable and it is the responsibility of cuml/cugraph to include those during their build?

actually there might be an even better/more sustainable way, since ExternalProject_Add can be used to trigger build processes of local folders (as opposed to just downloading/git cloning). This would allow the cmake code to be the same for working with a local or cloned RAFT, as well as triggering the build from cuML's cmake, let me do the change to see how it looks

@teju85 now both paths are using ExternalProject_Add so that if we need to eventually configure raft, or build raft (and statically link it or include it in the libcuml package) in the future, the build infra would support that.

Couldn't pull it from the if since the DOWNLOAD_COMMAND "" parameter overrides the git cloning no matter what :/

This is atleast better than the previous one. Thanks Dante for looking into this..

…ed in the future

teju85

Changes LGTM. We'll anyways be testing this follow thoroughly once we have raft handle created. Then we can make appropriate updates to this infra.

dantegd · 2020-04-29T03:53:14Z

@JohnZed forgot to request a rereview here from you

JohnZed

Looks good! Tiny suggestions added but preapproving too and happy to see those suggestions in a future PR

JohnZed · 2020-04-29T20:19:21Z

python/setup.py

-if not CUDA_HOME:
-    CUDA_HOME = (
+if not cuda_home:
+    cuda_home = (


Tiny thing - there is shutil.which for a more python native version of this

JohnZed · 2020-04-29T20:20:53Z

python/setuputils.py

+
+
+def get_environment_option(name):
+    ENV_VARIABLE = os.environ.get(name, False)


Probably shouldn’t be all caps

JohnZed · 2020-04-29T20:21:50Z

python/setuputils.py

+    """
+    Function to use the python code in RAFT in package.raft
+
+    - If RAFT symling already exists, don't change anything. Use setup.py clean


JohnZed · 2020-04-29T20:22:20Z

python/setuputils.py

+    - Otherwise it will look for RAFT in the libcuml build folder,
+        located either in the default location ../cpp/build or in
+        $CUML_BUILD_PATH.
+    -Otherwise it will clone RAFT into _external_repositories.


Missing space

dantegd · 2020-04-30T15:03:55Z

rerun tests

dantegd added 6 commits April 22, 2020 19:11

Merge branch-0.14 into 014-barge

c3fc152

Merge remote-tracking branch 'origin/branch-0.14' into 014-barge

1efa1cc

FEA Updates to use the current branch-0.14 of rapidsai RAFT

a7934f2

FEA Add test_raft file and test

40ed46b

FEA Make RAFT not be overwritten unles clean target is used

c73189b

FIX Correct python cloned repos name in gitignore

5e8a8ba

dantegd added 2 - In Progress Currenty a work in progress Build or Dep Issues related to building the code or dependencies labels Apr 23, 2020

dantegd requested review from a team as code owners April 23, 2020 01:35

dantegd requested review from JohnZed, cjnolet and teju85 April 23, 2020 01:38

DOC Added entry to changelog

0a37309

dantegd changed the title ~~[WIP] Build infrastructure to use RAFT~~ [REVIEW] Build infrastructure to use RAFT Apr 23, 2020

dantegd added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currenty a work in progress labels Apr 23, 2020

JohnZed requested changes Apr 24, 2020

View reviewed changes

dantegd added 4 - Waiting on Author Waiting for author to respond to review and removed 3 - Ready for Review Ready for review by team labels Apr 24, 2020

teju85 requested changes Apr 26, 2020

View reviewed changes

dantegd added 4 commits April 26, 2020 21:03

FIX PR Review feedback

3c0a987

DOC Updated python README.md

ea165fd

FIX Remove usage of relative paths

71eeff9

FIX Update RAFT pin

36cca3d

dantegd removed the 4 - Waiting on Author Waiting for author to respond to review label Apr 27, 2020

dantegd added the 4 - Waiting on Reviewer Waiting for reviewer to review or respond label Apr 27, 2020

dantegd changed the title ~~[REVIEW] Build infrastructure to use RAFT~~ [REREVIEW] Build infrastructure to use RAFT Apr 27, 2020

Merge branch 'branch-0.14' into 014-barge

46da22f

teju85 mentioned this pull request Apr 27, 2020

[BUG] We shouldn't expose our dependent library symbols across libcuml++.so #2138

Open

dantegd added 3 commits April 28, 2020 12:54

FEA Use externalproject_add to support config and build steps if need…

6a4338e

…ed in the future

Merge branch-0.14 into 014-barge

4319aa2

Merge conflict resolution

e9f288c

afender mentioned this pull request Apr 28, 2020

[FEA] Build infrastructure to use RAFT rapidsai/cugraph#839

Closed

teju85 approved these changes Apr 29, 2020

View reviewed changes

Merge branch 'branch-0.14' into 014-barge

11b142f

JohnZed approved these changes Apr 29, 2020

View reviewed changes

dantegd added 4 commits April 30, 2020 09:30

Merge branch-0.14 into 014-barge

bbff895

FIX Small corrections from PR review

c222d7d

FIX conflict resolution

60eb4be

DOC Changelog correction from bad merge

db88ca1

dantegd added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 4 - Waiting on Reviewer Waiting for reviewer to review or respond labels Apr 30, 2020

dantegd merged commit ddc3be2 into rapidsai:branch-0.14 May 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REREVIEW] Build infrastructure to use RAFT #2125

[REREVIEW] Build infrastructure to use RAFT #2125

dantegd commented Apr 23, 2020

GPUtester commented Apr 23, 2020

JohnZed left a comment

JohnZed Apr 24, 2020

dantegd Apr 27, 2020

teju85 Apr 26, 2020

dantegd Apr 26, 2020

dantegd Apr 26, 2020

teju85 Apr 27, 2020

teju85 Apr 26, 2020

dantegd Apr 26, 2020

dantegd Apr 26, 2020

teju85 Apr 27, 2020

dantegd Apr 27, 2020

dantegd Apr 28, 2020

teju85 Apr 29, 2020

teju85 left a comment

dantegd commented Apr 29, 2020

JohnZed left a comment

JohnZed Apr 29, 2020

JohnZed Apr 29, 2020

JohnZed Apr 29, 2020

JohnZed Apr 29, 2020

dantegd commented Apr 30, 2020

		return repo_path, repo_cloned


		def get_submodule_dependency(repo,


		# Only cloned if RAFT_PATH env variable is not defined

		if(DEFINED ENV{RAFT_PATH})



		def get_environment_option(name):
		ENV_VARIABLE = os.environ.get(name, False)

[REREVIEW] Build infrastructure to use RAFT #2125

[REREVIEW] Build infrastructure to use RAFT #2125

Conversation

dantegd commented Apr 23, 2020

GPUtester commented Apr 23, 2020

JohnZed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teju85 left a comment

Choose a reason for hiding this comment

dantegd commented Apr 29, 2020

JohnZed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantegd commented Apr 30, 2020