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

Remove CI Caching to preserve diskspace #2948

Merged
merged 2 commits into from
Jul 20, 2022

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Jul 20, 2022

Which issue does this PR close?

Closes #2947

Rationale for this change

CI is failing after running out of space; I am trying to reduce used space. I have also always suspected that the build caching is not particularly effective at reducing CI time

What changes are included in this PR?

  1. Stop trying to cache the output of target directory
  2. Change build job to use cargo check (rather than actually build binary output, just check compile errors)

Are there any user-facing changes?

Not only does CI now pass, it is actually faster 😆

Here are the tests for the last time CI passed on master: https://github.com/apache/arrow-datafusion/runs/7399255003?check_suite_focus=true

Notably:

  • "test hash collisions" took 25m25s in total
  • "test workspace" took 25m49s

Here are the tests on this PR: https://github.com/apache/arrow-datafusion/runs/7435755729?check_suite_focus=true

Notably

  • "test hash collisions" took 21m2s in total
  • "test workspace" took 22m47s

There may well be other things going on that made the tests take longer on master / previously but I think this data demonstrates this code change doesn't make things noticeably worse

@@ -46,36 +46,29 @@ jobs:
# and thus do not depend on the OS, arch nor rust version.
path: /github/home/.cargo
key: cargo-cache-
- name: Cache Rust dependencies
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 have never been convinced this caching is particularly effective so I am going to test what happens when I remove it...

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if something is wrong with the config 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I found this https://github.com/marketplace/actions/rust-cache which does perform some heuristics and cleaning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe what's wrong here is that the .cargo die is not cached?

https://doc.rust-lang.org/cargo/guide/cargo-home.html#caching-the-cargo-home-in-ci

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This cache is automatically keyed by:

the github [job_id](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_id),
the rustc release / host / hash,
the value of some compiler-specific environment variables (eg. RUSTFLAGS, etc), and
a hash of all Cargo.lock / Cargo.toml files found anywhere in the repository (if present).
a hash of all rust-toolchain / rust-toolchain.toml files in the root of the repository (if present).

Seems particularly relevant to why the basic cache wasn't working for us

@@ -107,12 +100,6 @@ jobs:
path: /github/home/.cargo
# this key equals the ones on `linux-build-lib` for re-use
key: cargo-cache-
- name: Cache Rust dependencies
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For what it is worth the last time this CI check passed on master was https://github.com/apache/arrow-datafusion/runs/7399254770?check_suite_focus=true where it took 24m and 26 seconds (after the linux-build-lib job finished)

@alamb alamb marked this pull request as ready for review July 20, 2022 19:05
@alamb alamb requested a review from andygrove July 20, 2022 19:09
@alamb alamb changed the title Test Remove CI Caching Remove CI Caching to preserve diskspace Jul 20, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #2948 (91c20f3) into master (944ef3d) will decrease coverage by 0.00%.
The diff coverage is 76.19%.

@@            Coverage Diff             @@
##           master    #2948      +/-   ##
==========================================
- Coverage   85.44%   85.43%   -0.01%     
==========================================
  Files         275      275              
  Lines       49690    49739      +49     
==========================================
+ Hits        42458    42495      +37     
- Misses       7232     7244      +12     
Impacted Files Coverage Δ
datafusion/common/src/scalar.rs 84.80% <ø> (-0.02%) ⬇️
datafusion/core/tests/sql/information_schema.rs 98.55% <ø> (ø)
datafusion/sql/src/planner.rs 81.26% <0.00%> (ø)
datafusion/core/src/execution/context.rs 77.95% <7.69%> (-1.12%) ⬇️
datafusion/core/src/catalog/information_schema.rs 94.67% <95.91%> (+0.95%) ⬆️
datafusion/expr/src/window_frame.rs 92.43% <0.00%> (-0.85%) ⬇️
datafusion/expr/src/logical_plan/plan.rs 77.14% <0.00%> (+0.17%) ⬆️

run: |
cargo build
cargo check
Copy link
Contributor

Choose a reason for hiding this comment

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

:👍

@ursabot
Copy link

ursabot commented Jul 20, 2022

Benchmark runs are scheduled for baseline = d11e28a and contender = b49093c. b49093c is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@alamb alamb deleted the alamb/remove_ci_caching branch July 20, 2022 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI test is failing with final link failed: No space left on device
4 participants