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

[REVIEW] Adding Ability to Set Arbitrary Cmake Flags in ./build.sh [skip-ci] #3144

Merged
merged 5 commits into from
Nov 19, 2020

Conversation

mdemoret-nv
Copy link
Contributor

@mdemoret-nv mdemoret-nv commented Nov 16, 2020

Closes #3127

This PR simply adds the environment variable $CUML_ADDL_CMAKE_ARGS to ./build.sh to allow developers to set arbitrary cmake flags on the command line. Below is a simple example:

CUML_ADDL_CMAKE_ARGS="-DBUILD_CUML_C_LIBRARY=OFF" ./build.sh

This would disable building the cuML C library which is hard coded to ON in ./build.sh.

Note: If cmake options are specified multiple times on the command line, the latter will override the former allowing this variable to work for options that are already specified in ./build.sh

@mdemoret-nv mdemoret-nv requested a review from a team as a code owner November 16, 2020 21:28
@GPUtester
Copy link
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@mdemoret-nv
Copy link
Contributor Author

mdemoret-nv commented Nov 16, 2020

@JohnZed Is this all you were looking for to close the linked issue? Or did you need something more?

Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a request for an example/docstring, otherwise this is great!

build.sh Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Nov 16, 2020

Codecov Report

Merging #3144 (4400f0e) into branch-0.17 (59497fa) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.17    #3144      +/-   ##
===============================================
- Coverage        70.57%   70.57%   -0.01%     
===============================================
  Files              197      197              
  Lines            15465    15465              
===============================================
- Hits             10915    10914       -1     
- Misses            4550     4551       +1     
Impacted Files Coverage Δ
...l/_thirdparty/sklearn/preprocessing/_imputation.py 62.09% <0.00%> (-0.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59497fa...4400f0e. Read the comment docs.

Copy link
Contributor

@JohnZed JohnZed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good just tiny naming thought

build.sh Outdated
@@ -74,6 +74,12 @@ BUILD_STATIC_FAISS=OFF
INSTALL_PREFIX=${INSTALL_PREFIX:=${PREFIX:=${CONDA_PREFIX}}}
PARALLEL_LEVEL=${PARALLEL_LEVEL:=""}

# Allow setting arbitrary cmake args via the $CUML_ADDL_CMAKE_ARGS variable. Any
# values listed here will override existing arguments. For example:
# CUML_ADDL_CMAKE_ARGS="-DBUILD_CUML_C_LIBRARY=OFF" ./build.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really picky thought but could we make it CUML_EXTRA_CMAKE_ARGS instead? like how in Make you have "EXTRA_CFLAGS" insteasd of ADDL_CFLAGS?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed it to CUML_EXTRA_CMAKE_ARGS as requested. There was also a similar variable for python build that I renamed to CUML_EXTRA_PYTHON_ARGS to match. See the help output for an example:

 The following environment variables are also accepted to allow further customization:
   PARALLEL_LEVEL         - Number of parallel threads to use in compilation.
   CUML_EXTRA_CMAKE_ARGS  - Extra arguments to pass directly to cmake. Values listed in environment
                            variable will override existing arguments. Example:
                            CUML_EXTRA_CMAKE_ARGS="-DBUILD_CUML_C_LIBRARY=OFF" ./build.sh
   CUML_EXTRA_PYTHON_ARGS - Extra argument to pass directly to python setup.py

@mdemoret-nv mdemoret-nv added the 4 - Waiting on Reviewer Waiting for reviewer to review or respond label Nov 17, 2020
Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm from my part

@mdemoret-nv mdemoret-nv 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 Nov 19, 2020
@JohnZed JohnZed changed the title [REVIEW] Adding Ability to Set Arbitrary Cmake Flags in ./build.sh [REVIEW] Adding Ability to Set Arbitrary Cmake Flags in ./build.sh [skip-ci] Nov 19, 2020
@JohnZed JohnZed merged commit d30edd9 into rapidsai:branch-0.17 Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Allow arbitrary CMake flags to be passed through ./build.sh
5 participants