-
Notifications
You must be signed in to change notification settings - Fork 194
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] [ENH] Ability to use ccache to speedup local builds #729
[REVIEW] [ENH] Ability to use ccache to speedup local builds #729
Conversation
NB: I meant to follow up on PR #651 , but kept postponing it :) |
Would it make sense to enable ccache by default if no arguments are passed to |
@achirkin I agree with Robert's comment in your PR above. I think this should be an option that should be enabled by the users when they want it. So, that's why this PR makes changes to the |
Sure, I totally agree. I've just put it here to link the discussion and to not forget to close that PR. (That still leaves open the question if it's ok to enable ccache when no arguments are passed to |
@@ -23,6 +23,7 @@ dependencies: | |||
- doxygen>=1.8.20 | |||
- libfaiss>=1.7.0 | |||
- faiss-proc=*=cuda | |||
- ccache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we do the same changes in cuml? I personally always use cuml_dev
environment for raft development, and I suspect that's not me only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with you. If this PR gets accepted by ops team, we could do the same in cuML too.
build.sh
Outdated
@@ -18,7 +18,7 @@ ARGS=$* | |||
# script, and that this script resides in the repo dir! | |||
REPODIR=$(cd $(dirname $0); pwd) | |||
|
|||
VALIDARGS="clean libraft pyraft pylibraft docs tests bench clean -v -g --install --compile-libs --compile-nn --compile-dist --allgpuarch --sccache --no-nvtx --show_depr_warn -h --buildfaiss --minimal-deps" | |||
VALIDARGS="clean libraft pyraft pylibraft docs tests bench clean -v -g --install --compile-libs --compile-nn --compile-dist --allgpuarch --sccache --ccache --no-nvtx --show_depr_warn -h --buildfaiss --minimal-deps" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I have recommended we switch over to a command line pattern like --cache-tool=<program>
the reason for this is the following:
- It allows a user to specify either sccache or ccache.
- Since they provide a name or path we don't require the command to be on the PATH
- It removes the need to check if both options are enabled
- It unifies the
hasArg --ccache
andhasArg --sccache
conditionals into a single branch - It allows people to also use cache tools like
distcc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach! Thanks Robert. Let me update the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robertmaynard can I get a re-review please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks amazing! Thank you
Co-authored-by: AJ Schmidt <ajschmidt8@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of the --ccache-tool
instances in build.sh
still need to be updated as mentioned in my comment below.
ha... I missed your suggestion. Thanks AJ for catching this. The latest commit above should address this now. |
@gpucibot merge |
In the nightly development docker image, the
today. At the end of the day, compilation still seemed not to hit the cache. These are the statistics that ccache reports: (note the
Does anybody know how to resolve this issue? The only thing I have been able to find is this ccache issue ccache/ccache#772. |
@ahendriksen I expect that the version of ccache you are using doesn't fully support the CUDA language. In Ubuntu 20.04 the ccache version is 3.7.7 and numerous issues with the CUDA language was backported to the 3 series in 3.7.9. I know locally ( and rapids-compose ) both use ccache 4.5+ for full CUDA support. |
@robertmaynard Thank you! That solved the issue completely. Cache is now hit 100% of the time and build time is down to 9 seconds. |
I noticed that we have support for
sccache
inbuild.sh
for building RAFT, but noccache
(especially if one wants to do local builds). This PR adds support for the latter. Also includes changes to installccache
in raft dev conda env.Let me know in case you have any concerns with adding this.