[BUILD] Add caching to CMake #8373

electriclilies · 2021-06-29T23:46:41Z

In this PR I add code to cmake/config.cmake and CMakeLists.txt to cache builds. This makes switching between branches less painful because you don't have to build from scratch every time you switch branches.

@mbrookhart @tkonolige @csullivan Please take a look!

comaniac

LGTM. One tip: ccache-3.x doesn't support nvcc well, so CUDA kernels may never hit the cache and still need to be re-compiled every time. Using ccache 4.0+ can resolve this issue. Since Ubuntu 20 still uses ccache 3.x, it's hightly possible that users may need to manually install 4.0+.

mbrookhart · 2021-06-30T00:16:23Z

LGTM. One tip: ccache-3.x doesn't support nvcc well, so CUDA kernels may never hit the cache and still need to be re-compiled every time. Using ccache 4.0+ can resolve this issue.

Can we add that to the documentation on the option?

mehrdadh · 2021-06-30T00:37:19Z

@electriclilies this is great! thanks for adding this feature.

electriclilies · 2021-06-30T00:43:24Z

@mbrookhart Added it :)
@mehrdadh This snippet has been circulating for a while, I got it from @csullivan originally :) I figured I should put it in the codebase after it got wiped from my local config.cmake file when I built using docker and then I had to track down the snippet again 🤣

electriclilies · 2021-06-30T01:04:11Z

This PR has failed a few times on the windows build. I'm not sure if this is caused by this PR or not; we should keep an eye on it and perhaps set the default behavior to not cache if it is causing flaky tests.

junrushao · 2021-06-30T04:03:21Z

Thanks Lily for the great work! Yeah I am using ccache a lot as well when working on the TVM project, but with a more global flag, like

set(CMAKE_C_COMPILER_LAUNCHER /path/to/ccache)
set(CMAKE_CXX_COMPILER_LAUNCHER /path/to/ccache)
set(CMAKE_CUDA_COMPILER_LAUNCHER /path/to/ccache)

What's the difference between your approach (set_property(GLOBAL PROPERTY RULE_LAUNCH_COMPILE ) and the one above?

tqchen · 2021-06-30T12:58:58Z

Thanks @electriclilies . One thing that might be helpful here is to streamline the option definition. The current proposed option is closer to AUTO mode(enable if found, but disable if not).

Here are how we interpret values to keep things consistent with our previous options:

AUTO: automatically detect and if found use it
/path/to/ccache: set the ccache to the correct ccache path being specified
OFF: disable ccache
ON: search and report an error if ccache is not found

We should also look a bit into which way is more standard, since I also see set(CMAKE_C_COMPILER_LAUNCHER /path/to/ccache) being used a bit more frequently.

The use of ccache will likely cause the history cache growing problem in the CI when the node disk space is limited. Because the cost of build is normally smaller compared to testing, we will likely want to disable ccache in the CI setups in the task_config_build_xyz.sh https://github.com/apache/tvm/blob/main/tests/scripts/task_config_build_arm.sh

mehrdadh · 2021-06-30T16:17:13Z

I agree with @tqchen about the CI. It's better to always build from scratch in CI.

electriclilies · 2021-06-30T19:24:11Z

Thanks for the feedback @tqchen @junrushao1994. I'll change the default to AUTO, as suggested, and disable this option in CI.
Regarding which is more common-- I don't know which approach is more commonly used. One advantage to the approach I propose is that the user doesn't have to enter the path to the ccache, so they can take advantage of it without editing the config.cmake file.

tqchen · 2021-06-30T20:01:10Z

Thanks @electriclilies , by standard options, I actually meant we should implement the auto feature(please see the suggested comments) via

set(CMAKE_C_COMPILER_LAUNCHER /path/to/ccache) vs set_property(GLOBAL PROPERTY RULE_LAUNCH_COMPILE /path/to/ccache)

seems they are two ways to do the same thing(see some related issues Tencent/rapidjson#1794 seems to indicate that we should use the other one), but we are not sure

tqchen · 2021-06-30T23:18:15Z

CMakeLists.txt

+  find_program(CCACHE_FOUND ccache)
+  if(CCACHE_FOUND)
+    message(STATUS "Enabling ccache")
+    set_property(GLOBAL PROPERTY RULE_LAUNCH_COMPILE ccache)


would be grgeat to check if we can use set CXX_COMPILER_LAUNCHER here and get the same goal

tqchen · 2021-06-30T23:18:42Z

CMakeLists.txt

+
+if(USE_CCACHE)
+  find_program(CCACHE_FOUND ccache)
+  if(CCACHE_FOUND)


We might want to feed the result of find_program to the variable, in case ccache is not in path but find_program manages to find it

electriclilies · 2021-07-01T19:37:58Z

After some thought, I think that it is actually better to model the options in this case after the options for ROCM runtime, which looks like this:

# Whether enable ROCM runtime
#
# Possible values:
# - ON: enable ROCM with cmake's auto search
# - OFF: disable ROCM
# - /path/to/rocm: use specific path to rocm
set(USE_ROCM OFF)

How does this sound for the possible options?

# Possible values:
# - ON: enable ccache by searching for the path to ccache
# - OFF: disable ccache
# - /path/to/ccache: use specific path to ccache

tqchen · 2021-07-01T22:14:12Z

I agree, in the case of rocm runtime, an error will be raised if rocm is ON and we did not found the runtime. So the current behavior is still closer to AUTO.

# Possible values:
# - AUTO: search for path to ccache, disable if not found.
# - ON: enable ccache by searching for the path to ccache, report an error if not found
# - OFF: disable ccache
# - /path/to/ccache: use specific path to ccache

electriclilies · 2021-07-07T21:00:35Z

@tqchen @junrushao1994 I updated the options and changed the method of setting the path to ccache to set(CXX_COMPILER_LAUNCHER /path/to/ccache). I'm not sure if you meant that I need to do that in addition to set(CMAKE_C_COMPILER_LAUNCHER /path/to/ccache).
Let me know what you think

tkonolige · 2021-07-07T21:28:09Z

CMakeLists.txt

+    message(STATUS "Setting ccache path to " "${PATH_TO_CCACHE}")
+  endif()
+  # Set the flag for ccache
+  set(CXX_COMPILER_LAUNCHER PATH_TO_CCACHE)


You should probably check if these are already set so you don't override user options.

How do I do that? Also if it is already set, do I just not set it? Or somehow try to concatenate user input with the path to ccache?

In our specific case I think overriding CXX_COMPILER_LAUNCHER should not be a bad expected behavior, so we can leave it as it is for now

tqchen

Thanks @electriclilies ! One last minor comment and we are good to go

tqchen · 2021-07-14T16:46:50Z

CMakeLists.txt

+    else()
+      message(STATUS "Didn't find the path to CCACHE, disabling ccache")
+    endif(CCACHE_FOUND)
+  elseif("${USE_CCACHE}" STREQUAL "ON")


change to use

tvm/cmake/utils/Utils.cmake

Line 86 in 8131364

set(IS_TRUE_PATTERN "^[Oo][Nn]$|^[1-9][0-9]*$|^[Tt][Rr][Uu][Ee]$|^[Yy][Ee][Ss]$|^[Yy]$")

see

tvm/cmake/utils/FindEthosN.cmake

Line 64 in 8131364

if(${USE_ETHOSN_HW} MATCHES ${IS_TRUE_PATTERN})

@tqchen Done!

Fix formatting Add comment about nvcc Change default to AUTO More progress Add auto as a mode Disable ccache in CI add-cache-to-cmake Fix typo

electriclilies · 2021-07-30T22:05:19Z

@tqchen @junrushao1994 This is passing, could you take a look and make sure I addressed all the comments?

junrushao · 2021-07-30T22:43:54Z

I think the PR is in pretty good state. Just one question: Is it intended behavior to disable ccache in our CI? I thought we are using it right now

electriclilies · 2021-07-30T23:45:41Z

@junrushao1994

@tqchen said:

The use of ccache will likely cause the history cache growing problem in the CI when the node disk space is limited. Because the cost of build is normally smaller compared to testing, we will likely want to disable ccache in the CI setups in the task_config_build_xyz.sh https://github.com/apache/tvm/blob/main/tests/scripts/task_config_build_arm.sh

Based on this, it is my understanding that we do want to disable ccache in CI. I'm not sure whether we are using it right now. I think it would be good to double check that this is the intended behavior before it goes in given all the issues we've had with CI in the last week 😎

electriclilies · 2021-07-30T23:49:10Z

@tqchen @junrushao1994 Additionally, if I do make this change and we don't update the CI docker images, will ccache just be enabled on CI until the docker images are updated? That might be suboptimal if it does cause the memory usage to go up
cc @mbrookhart

junrushao · 2021-07-31T01:31:16Z

Thanks Lily for the explanation! The PR looks good to me :-)

* ccache * ccache Fix formatting Add comment about nvcc Change default to AUTO More progress Add auto as a mode Disable ccache in CI add-cache-to-cmake Fix typo * Fix rebase * flaky test

comaniac approved these changes Jun 30, 2021

View reviewed changes

mbrookhart approved these changes Jun 30, 2021

View reviewed changes

tqchen changed the title ~~Add caching to CMake~~ [Build] Add caching to CMake Jun 30, 2021

tqchen changed the title ~~[Build] Add caching to CMake~~ [BUILD] Add caching to CMake Jun 30, 2021

tqchen reviewed Jun 30, 2021

View reviewed changes

tkonolige reviewed Jul 7, 2021

View reviewed changes

tqchen reviewed Jul 14, 2021

View reviewed changes

tqchen added the status: need update need update based on feedbacks label Jul 16, 2021

electriclilies requested review from areusch, icemelon, jroesch, junrushao, merrymercy, yzhliu and a team as code owners July 20, 2021 18:20

tqchen approved these changes Jul 20, 2021

View reviewed changes

junrushao approved these changes Jul 20, 2021

View reviewed changes

electriclilies force-pushed the add-cache-to-cmake branch from dfc9ed4 to 7b76703 Compare July 26, 2021 17:24

electriclilies added 3 commits July 27, 2021 11:02

ccache

80dd39e

ccache

ba94de4

Fix formatting Add comment about nvcc Change default to AUTO More progress Add auto as a mode Disable ccache in CI add-cache-to-cmake Fix typo

Fix rebase

dbd8782

electriclilies force-pushed the add-cache-to-cmake branch from 70280c4 to dbd8782 Compare July 27, 2021 18:02

flaky test

13ad055

junrushao approved these changes Jul 31, 2021

View reviewed changes

junrushao merged commit 9f29e2a into apache:main Aug 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUILD] Add caching to CMake #8373

[BUILD] Add caching to CMake #8373

electriclilies commented Jun 29, 2021

comaniac left a comment •

edited

Loading

mbrookhart commented Jun 30, 2021

mehrdadh commented Jun 30, 2021

electriclilies commented Jun 30, 2021

electriclilies commented Jun 30, 2021

junrushao commented Jun 30, 2021

tqchen commented Jun 30, 2021 •

edited

Loading

mehrdadh commented Jun 30, 2021

electriclilies commented Jun 30, 2021 •

edited

Loading

tqchen commented Jun 30, 2021 •

edited

Loading

tqchen Jun 30, 2021

tqchen Jun 30, 2021

electriclilies commented Jul 1, 2021

tqchen commented Jul 1, 2021 •

edited

Loading

electriclilies commented Jul 7, 2021 •

edited

Loading

tkonolige Jul 7, 2021

electriclilies Jul 7, 2021 •

edited

Loading

tqchen Jul 31, 2021

tqchen left a comment

tqchen Jul 14, 2021

tqchen Jul 14, 2021

electriclilies Jul 20, 2021

electriclilies commented Jul 30, 2021

junrushao commented Jul 30, 2021

electriclilies commented Jul 30, 2021

electriclilies commented Jul 30, 2021 •

edited

Loading

junrushao commented Jul 31, 2021

[BUILD] Add caching to CMake #8373

[BUILD] Add caching to CMake #8373

Conversation

electriclilies commented Jun 29, 2021

comaniac left a comment • edited Loading

Choose a reason for hiding this comment

mbrookhart commented Jun 30, 2021

mehrdadh commented Jun 30, 2021

electriclilies commented Jun 30, 2021

electriclilies commented Jun 30, 2021

junrushao commented Jun 30, 2021

tqchen commented Jun 30, 2021 • edited Loading

mehrdadh commented Jun 30, 2021

electriclilies commented Jun 30, 2021 • edited Loading

tqchen commented Jun 30, 2021 • edited Loading

tqchen Jun 30, 2021

Choose a reason for hiding this comment

tqchen Jun 30, 2021

Choose a reason for hiding this comment

electriclilies commented Jul 1, 2021

tqchen commented Jul 1, 2021 • edited Loading

electriclilies commented Jul 7, 2021 • edited Loading

tkonolige Jul 7, 2021

Choose a reason for hiding this comment

electriclilies Jul 7, 2021 • edited Loading

Choose a reason for hiding this comment

tqchen Jul 31, 2021

Choose a reason for hiding this comment

tqchen left a comment

Choose a reason for hiding this comment

tqchen Jul 14, 2021

Choose a reason for hiding this comment

tqchen Jul 14, 2021

Choose a reason for hiding this comment

electriclilies Jul 20, 2021

Choose a reason for hiding this comment

electriclilies commented Jul 30, 2021

junrushao commented Jul 30, 2021

electriclilies commented Jul 30, 2021

electriclilies commented Jul 30, 2021 • edited Loading

junrushao commented Jul 31, 2021

comaniac left a comment •

edited

Loading

tqchen commented Jun 30, 2021 •

edited

Loading

electriclilies commented Jun 30, 2021 •

edited

Loading

tqchen commented Jun 30, 2021 •

edited

Loading

tqchen commented Jul 1, 2021 •

edited

Loading

electriclilies commented Jul 7, 2021 •

edited

Loading

electriclilies Jul 7, 2021 •

edited

Loading

electriclilies commented Jul 30, 2021 •

edited

Loading