ci: restore-keys for the LLBC caches, and two jobs' worth of duplicated work removed - #1347
Conversation
The `cpython-tests` job repeated the checkout, toolchain install, Cargo cache restore and Charon/LLBC download that `cargo-test-linux` already does on the same runner image, and needs the same prepared artifacts. Its two steps move into that job's shared step anchor, guarded on `runner.os == 'Linux'` so the macOS and Windows copies skip them, and the job is deleted. Both steps also carry `!cancelled()`, so a failing cargo pass above them no longer suppresses the suite the way plain step ordering would; as separate jobs neither could hide the other. `cargo-test-linux` takes the deleted job's `timeout-minutes`, raised to 90 to cover the work now in it. `pyre/cpython_tests/README.md` and the `--cpython-suite` docstring in `pyre/check.py` are repointed. Assisted-by: Claude
The step sat ahead of `Run pyre/check.py` on the premise that building `--features web` first would leave check.py's later `--features wasm-host` build sharing its compiled dependencies. Measured on a cold target dir: the web build compiles 328 crates in 6m00 and leaves the wasm-host build compiling 166 instead of 323 — 1m42 against a 2m32 cold control. Half the graph still recompiles because check.py sets `RUSTFLAGS='-C link-arg=--export-table -C link-arg=--growable-table --cfg getrandom_backend="custom"'` and the web step sets none. On the ubuntu runner the step measures 11.7 min and the saving it buys is worth about 1.7. It moves to `sandbox-build`, which already has the checkout, toolchain, Cargo cache and prepared Charon/LLBC set it needs, is on the same runner image, and at 15.4 min sits far off the run's critical path. It goes first in that job so the compile signal still arrives early. Nothing executes the artefact either way; check.py writes its own pyre_wasm.wasm. That job is renamed to `sandbox e2e + wasm web build (ubuntu-24.04)`. Assisted-by: Claude
The four entries carried an exact key and nothing else, so a miss restored nothing. The key is the wide digest — `Compute LLBC fingerprints` runs before any `.readfiles` exists, so `source=` is the whole cargo closure there and one edit anywhere misses all four. Every run therefore re-extracted the whole set: 10.6 min on ubuntu, 19.5 on windows, 22.3 on macOS. With a `restore-keys` prefix a miss falls back to the crate's most recent entry, and `extract-llbc.py` adjudicates: it recomputes each crate's stamp against the current tree, narrowing `source=` through the restored `.readfiles`, and compares it to the restored stamp as exact text. Verified locally — a comment appended to `pyre/pyre-jit/src/call_jit.rs`, which is in pyre-jit's readfiles and no other crate's, re-extracts pyre-jit alone and skips the other three; with nothing changed all four skip in 2s. The key therefore only chooses what is offered. A stale restore costs a download and is re-extracted, and cannot reach the artefact. Measured shares of one extraction: pyre-interpreter 376s, pyre-jit 106s, pyre-object 53s, majit-rlib 40s. Over the last 60 commits on main the share of commits touching no file in a crate's readfiles is 48%, 33%, 73% and 92% respectively, and 8% of commits touch all four. Weighting the first by the second puts the expected saving near half of each Extract step. Assisted-by: Claude
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. WalkthroughThe CI workflow adds per-crate LLBC cache fallbacks, integrates CPython tests into Linux cargo testing, removes the standalone CPython job, and moves the wasm web build into the sandbox job. Related documentation names the new CPython job. ChangesCI workflow updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The PR improves CI cache reuse and removes duplicated build and test setup without introducing an actionable merge-blocking risk; it is merge-ready after normal checks. Sequence Diagram(s)sequenceDiagram
participant cargo-test-linux
participant CPython-3.14
participant CPython-suite
participant sandbox-e2e
cargo-test-linux->>CPython-3.14: conditional setup on Linux
cargo-test-linux->>CPython-suite: build and run release regression suite
CPython-suite-->>cargo-test-linux: return parallel test results
sandbox-e2e->>sandbox-e2e: install wasm32 target
sandbox-e2e->>sandbox-e2e: build pyre-wasm web feature
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ee4f5643d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| restore-keys: | | ||
| llbc2-majit-rlib-${{ runner.os }}-${{ runner.arch }}-${{ env.CHARON_VERSION }}- |
There was a problem hiding this comment.
On every primary-key miss caused by a closure edit, this fallback cannot produce the advertised per-crate skip: Compute LLBC fingerprints calculates source= without a .readfiles sidecar, so it falls back to the same conservative input set hashed into the cached stamp's closure= field. Therefore a changed primary digest also means the restored entry's closure= differs from the current stamp, and extract()'s byte-for-byte stamp comparison rejects it and re-extracts the crate. The new fallback consequently downloads a stale LLBC on each miss without avoiding extraction; the key or freshness gate needs to be aligned so a fallback can actually pass.
AGENTS.md reference: AGENTS.md:L67-L72
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 5ee4f56). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptationsNone. |
1 participant
Three CI changes, measured rather than guessed. Baseline is run
32172685070(all jobs green): 373 min of wall across 13 jobs, or 1363 min once the
macOS ×10 and Windows ×2 multipliers are applied, with the critical path at
78 min (
prepare Charon/LLBC (ubuntu)11.6 →pyre/check.py (ubuntu)66.7).1. The per-crate LLBC caches get a
restore-keysfallbackThe four entries carried an exact key and nothing else, so a miss restored
nothing. The key is the wide digest —
Compute LLBC fingerprintsrunsbefore any
.readfilesexists, sosource=falls back to the whole cargoclosure there, and one edit anywhere misses all four crates. Every run
re-extracted the whole set: 10.6 min ubuntu, 19.5 windows, 22.3 macOS.
This is safe because the key is only a hint.
extract-llbc.pyrecomputes eachcrate's stamp against the current tree — narrowing
source=through therestored
.readfiles— and compares it to the restored stamp as exact textover
source=/closure=/external=/charon=/layout_flags=. A restoredartefact that does not match is re-extracted, so a stale restore costs a
download and cannot reach the artefact.
Verified locally:
skipping … (fingerprint unchanged)in 2spyre/pyre-jit/src/call_jit.rs, which is in pyre-jit'sreadfiles and no other crate's → pyre-jit alone re-extracts, the other
three skip
Expected saving, from measured per-crate extraction times against the share of
the last 60 commits on
mainthat touch no file in each crate's readfiles:All four must re-extract on 8% of commits; weighting share by hit rate puts the
expected saving near half of each Extract step.
2. The
pyre-wasmweb-binding build moves off the check.py legIt sat ahead of
Run pyre/check.pyon the premise that building--features webfirst would leave check.py's later--features wasm-hostbuild sharing compiled dependencies. Measured on a cold target dir:
build --features webcoldbuild --features wasm-hostafter itbuild --features wasm-hostcold, aloneThe sharing is real and worth 50 seconds for 6 minutes spent — half the
graph still recompiles because check.py sets
RUSTFLAGS='-C link-arg=--export-table -C link-arg=--growable-table --cfg getrandom_backend="custom"'and the web step sets none. On the ubuntu runner the step measures 11.7 min.
The build itself is unchanged — this keeps the full
cargo buildcompilesignal, it just runs somewhere cheaper.
sandbox-buildalready carries thecheckout, toolchain, Cargo cache and prepared Charon/LLBC set it needs, runs on
the same image, and at 15.4 min sits far off the critical path. It goes first in
that job so the signal still arrives early. Nothing executes the artefact either
way; check.py writes its own
pyre_wasm.wasm. The job is renamedsandbox e2e + wasm web build (ubuntu-24.04).3. The CPython suite becomes trailing steps of
cargo test (ubuntu-24.04)cpython-testsrepeated the checkout, toolchain install, Cargo cache restoreand Charon/LLBC download that
cargo-test-linuxalready does on the same runnerimage, and needed the same prepared artefacts. Its two steps move into that
job's shared step anchor under
runner.os == 'Linux', and the job is deleted.Both carry
!cancelled(), so a failing cargo pass above them cannot suppressthe suite the way plain step ordering would — as separate jobs neither could
hide the other.
cargo-test-linuxtakes the deleted job'stimeout-minutes,raised to 90 to cover the work now in it.
This one is the smallest of the three: it recovers about 1.4 min of duplicated
setup and one runner slot. It does not remove the release
pyre-dynasmbuild, which check.py still does separately.
Expected effect
What to watch on the first run
sandbox-buildhas noFree up runner disk spacestep and now links a wasm32release target on top of a native release
pyreand a release e2e test binary.If disk runs out there, that step can be copied across from
cargo-test-linux.Checked and deliberately not changed
Diffing
llbc-Linux-X64againstllbc-macOS-ARM64from one run,pyre-interpreter.ullbc.readfilesdiffers: macOS listsinterp_kevent.rs,interp_kqueue.rsandselect/mod.rs, which Linux never compiles. Theartefacts describe different sets of functions, so the per-OS artefact naming
is load-bearing. Only the
*.wasm32-unknown-unknown.layouts.ullbcsidecarsare byte-identical across hosts.
cargo test (ubuntu-24.04)on main reportsRestored from cache key "v0-rust-X64-Linux-x64-…" full match: true,Cache Size: ~2628 MB. The 0.00mPost Runsteps are an exact hit correctlyskipping the save, not a broken cache.
timestamps in the check.py job logs, spawn overhead outside the measured
seconds is 1.9 min on ubuntu (0.05s across 2149 spawns), 2.6 on windows, 1.6
on macOS.
cargo testwhen no Rust changed would never fire — 0 of thelast 40 commits on
mainleave*.rs/Cargo.toml/Cargo.lockuntouched.Summary by CodeRabbit
Bug Fixes
Tests
Documentation