-
Notifications
You must be signed in to change notification settings - Fork 131
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
Increase runner size for Cargo Dev and Sanitizer workflows #685
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Reviewable status: 0 of 2 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / large-ubuntu-22.04, Cargo Dev / macos-13, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, Vercel, asan / large-ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, publish-image, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04 (waiting on @allada and @MarcusSorealheis)
7d5873d
to
ab7202d
Compare
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.
Reviewable status: 0 of 2 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Bazel Dev / ubuntu-22.04, Local / ubuntu-22.04, Vercel, asan / large-ubuntu-22.04, docker-compose-compiles-nativelink (20.04), pre-commit-checks, publish-image, ubuntu-20.04 / stable, ubuntu-22.04, vale (waiting on @MarcusSorealheis)
a discussion (no related file):
I'm not sold we should do this. We are not doing complex stuff in these runners, why is it using so much disk size?
Last time we increased runner sizes it cost a ton of money too.
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.
Reviewable status: 0 of 2 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Remote / large-ubuntu-22.04, asan / large-ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, publish-image, windows-2022 / stable (waiting on @allada and @MarcusSorealheis)
a discussion (no related file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
I'm not sold we should do this. We are not doing complex stuff in these runners, why is it using so much disk size?
Last time we increased runner sizes it cost a ton of money too.
The sanitizer run produces much larger artifacts since it builds instrumented code. That run was somewhat expected to run into issues at some point.
The cargo-dev run runs into issues because the default size for the linux runners is a bit smaller than Mac and Windows, so this is also not too surprising.
I agree that we don't really need more cores though. Maybe we could add a new runner type in github that just increases the local storage size but doesn't add additional cores?
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.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @MarcusSorealheis)
a discussion (no related file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
The sanitizer run produces much larger artifacts since it builds instrumented code. That run was somewhat expected to run into issues at some point.
The cargo-dev run runs into issues because the default size for the linux runners is a bit smaller than Mac and Windows, so this is also not too surprising.
I agree that we don't really need more cores though. Maybe we could add a new runner type in github that just increases the local storage size but doesn't add additional cores?
It looks like these runners have 14 GB
according to documentation.
I'm thinking maybe sanitizers should use a non-debug build? I believe cargo by default uses debug builds.
Could we also somehow tell nix to use less disk space?
ab7202d
to
870c938
Compare
870c938
to
21252c6
Compare
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.
Reviewable status: 0 of 2 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, Vercel, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, publish-image, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04 (waiting on @allada and @MarcusSorealheis)
a discussion (no related file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
It looks like these runners have
14 GB
according to documentation.I'm thinking maybe sanitizers should use a non-debug build? I believe cargo by default uses debug builds.
Could we also somehow tell nix to use less disk space?
Removing debug symbols seems to help xD
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.
Reviewed 2 of 3 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @MarcusSorealheis)
.github/workflows/nix.yaml
line 84 at r4 (raw file):
run: > nix develop --impure --command bash -c "cargo test --all --release"
I suggest adding this to Cargo.toml
:
[profile.tiny]
inherits = "release"
opt-level = "z"
strip = true
lto = "thin"
Then use --profile=tiny
instead. I think this will dramatically improve compile time too.
Introduce `smol` profile for smaller, faster builds. The new profile reduces the size of the `target` directory from ~12GB to ~1GB. Fixes recent CI breakages due to "no space left on device" errors.
21252c6
to
66a9002
Compare
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.
Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, publish-image, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04 (waiting on @MarcusSorealheis)
Cargo.toml
line 15 at r5 (raw file):
# Prefer this profile in CI, for instance via `cargo test --all --profile=smol`. # It reduces the size of the `target` directory from ~12GB to ~1GB. [profile.smol]
nit: what does smol
mean?
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.
Reviewable status: 1 of 2 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, publish-image, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04 (waiting on @MarcusSorealheis)
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.
Reviewable status: 1 of 1 LGTMs obtained
Cargo.toml
line 15 at r5 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: what does
smol
mean?
🐈 https://knowyourmeme.com/memes/smol
See also:
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.
Reviewed all commit messages.
Reviewable status: 1 of 1 LGTMs obtained
This should fix recent CI breakages on main.
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)