fix(ci): cache the tools_core wheel so the required lane stops re-downloading it - #4487
Merged
Conversation
…nloading it The `tests` lane is the required check, and its most frequent failure is not a test but a download. Every run installed the Rust toolchain via rustup, fetched the crates.io registry into a fresh per-workspace CARGO_HOME, force-reinstalled maturin from PyPI with `--no-cache-dir`, and recompiled the wheel — before running a single test. Measured across the 2026-08-13 consolidation drive, that was the largest single source of red required checks: of ~20 job failures on the consolidation PRs, every one was a network step, and the Rust toolchain / wheel build led the tally. Two PRs failed at the identical step (#16) on two different runners minutes apart. None of those failures had anything to do with the change under test. Most pull requests do not touch `rust_core/`, so the wheel they need is byte-identical to the last one built. This caches the built wheel keyed on everything that can change its contents or ABI — `rust-toolchain.toml`, the workspace `Cargo.toml`/`Cargo.lock` (tools_core is a workspace member), the crate's own manifests, and its sources — plus `runner.os` and the Python version because the wheel is ABI-specific. On a hit the toolchain install, the crate downloads and the compile are all skipped. On a miss it also caches the cargo registry and target directory, mirroring the block already in `maturin-ai-backend.yml` so the repo's two Rust builds cache the same way. Three details worth review: - A cache hit is not proof of a usable wheel. If an earlier build failed after creating the output directory, the entry can exist but be empty, and `actions/cache` will not overwrite a key that already matched. So the decision to build reads the filesystem rather than `cache-hit`: a hit with no wheel falls back to building, which is today's behaviour and never worse than it. - Install and verification now run on both paths, so the `import tools_core` check still gates every run. A poisoned cache fails loudly there rather than silently shipping an unusable extension module. - maturin is left unpinned. Dropping `--force-reinstall --no-cache-dir` is what stops the guaranteed fresh download; every other maturin call site in this repo uses a plain `pip install maturin`, and choosing a version is a separate reviewed decision rather than a drive-by change inside a caching fix. Scoped to the required `tests` lane deliberately. `rust-quality-gate` builds the wheel the same way and would benefit identically, but it is not a required check and keeping this diff reviewable matters more than covering both at once.
![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: 8e261bfd80
ℹ️ 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".
Comment on lines
+556
to
+559
| hashFiles('rust-toolchain.toml', 'Cargo.toml', 'Cargo.lock', | ||
| 'rust_core/tools-core/Cargo.toml', | ||
| 'rust_core/tools-core/pyproject.toml', | ||
| 'rust_core/tools-core/src/**/*.rs') }} |
There was a problem hiding this comment.
When a PR changes rust_core/math-primitives, this key remains unchanged because tools-core consumes it as a local path dependency and such source edits do not alter Cargo.lock. The required Python 3.11 lane therefore restores the old wheel, skips the build, and runs the Rust-binding tests against stale code; after merge, the exact cache hit can keep that stale wheel in use indefinitely. Hash the dependency's Cargo.toml and src/**/*.rs files along with the tools-core inputs.
Useful? React with 👍 / 👎.
| path: | | ||
| .cargo-home/registry | ||
| .cargo-home/git | ||
| rust_core/tools-core/target |
There was a problem hiding this comment.
On every wheel-cache miss, this path does not contain the compilation artifacts: tools-core belongs to the root Cargo workspace, whose metadata reports ${{ github.workspace }}/target as its target directory (the existing rust-quality-gate likewise installs its wheel from root target/wheels). Consequently this cache only preserves the registry and Git downloads and every invalidated wheel recompiles all dependencies. Cache target here or set CARGO_TARGET_DIR to the configured crate-local path.
Useful? React with 👍 / 👎.
dieterolson
enabled auto-merge (squash)
August 15, 2026 15:05
Follow-up to the first version of this change, driven by what the first CI run actually did rather than by what it was supposed to do. On job 95025824208 all five new cache steps succeeded and the wheel built cleanly -- then the job failed downstream at `Collect Changed Coverage Inputs` (a `git fetch`). Because `actions/cache` only writes in a post-job step, that post step was skipped and the freshly built wheel was discarded: the cache list still holds no `toolscore-*` entry at all. On a fleet where downstream network steps fail routinely, a save-at-job-end cache would almost never populate, which defeats the entire point of caching it. Changes: - `actions/cache/restore@v6` plus an explicit `actions/cache/save@v6` placed immediately after a successful build, so one good build benefits every later run even if that run goes on to fail for unrelated reasons. - The cache key is computed once into a step output. A six-file `hashFiles` expression duplicated across a restore and a save is a silent cache-never-hits bug waiting to happen; now they cannot drift. - The save is `continue-on-error: true`. It is an optimisation and must never be able to fail the one required lane in this repo -- two jobs that both miss will both try to reserve the same key and the loser errors, which is a cache miss next run, not a broken build. Verified `actions/cache` v6 really does ship `save/action.yml` and `restore/action.yml` before depending on them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…w loading My previous commit computed the cache key once into a step output so the restore and the save could not drift. That is the better shape, but it is not expressible here: `hashFiles` is not valid inside a `run:` block, and putting it there made `ci-standard.yml` fail to load entirely. The failure mode is worth recording because it does not look like a workflow error. Run 31894981880 reported `status=completed conclusion=failure` with **zero jobs created**, and its name rendered as the raw path `.github/workflows/ ci-standard.yml` instead of "CI Standard". `gh pr checks` therefore listed no failing check at all -- the required lane was simply absent, which reads as "still queued" rather than "broken". The tell is a completed run with an empty jobs array: that means the workflow never loaded, so the fault is in the file, not in any job. `gh run view` confirms: "This run likely failed because of a workflow file issue." Both keys are now the same folded expression that loaded successfully in the first revision, verified byte-identical after whitespace folding. GitHub Actions has no YAML anchors, so the duplication cannot be factored out; a comment on each step is the guard against drift. Verified before pushing, rather than after: `scripts/validate_workflows.py`, `scripts/check_workflow_pinning.py`, and `scripts/check_blocking_quality_gates.py` all pass, no `hashFiles` remains in any `run:` block in any job, and no reference to the removed step output survives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dieterolson
added a commit
that referenced
this pull request
Aug 16, 2026
…nder the size budget Merges origin/main (now carrying #4448 and #4487) into the 34-PR variation / Morris consolidation, and clears the two gates that were failing locally. SPEC.md was the only conflict: one hunk in the newest-first Change Log table. Resolved ours-then-theirs. Both sides insert 2026-08-13 rows, so the date does not decide the order -- what decides it is that main's row is 1.5.7 and the row immediately AFTER the conflict is 1.5.6. That adjacency is main's own version sequence, so main's row has to stay last or the merge silently reorders main's changelog. Verified after resolving: main's 1.5.8 and 1.5.7 rows both present, 1.5.7 still immediately above the 2026-08-13 1.5.6 rows. Note this is the OPPOSITE order from the sibling consolidations' SPEC conflicts, which were theirs-then-ours. Same underlying rule in both cases -- the trailing side belongs with whatever follows the conflict -- but it resolves to a different order depending on whether that trailing content is a table row or a section bullet. Neither can be settled by a blanket "prefer ours" or `--union`. Formatting: 4 files under src/data_processing/data_processor/python/ were still carrying the older style this stack had reverted. Reformatted with the CI-pinned ruff, so `ruff format --check` is clean across all 2976 files. File-size budget: 2 violations, handled differently on purpose. - src/shared/python/swing_sim/variation/execution_metadata.py (525 LOC) is SPLIT, not grandfathered. Its schema identifiers and field frozensets are declarations with no behaviour, so they moved cleanly into `_execution_metadata_schema.py` (75 LOC) and are re-exported, leaving every existing importer unaffected. 525 -> 488 LOC. Verified all 15 moved names still resolve through `execution_metadata` after `ruff --fix` ran over the re-export block, since an import-and-re-export pattern is exactly what an unused-import autofix is liable to strip. - src/rate_of_closure/ui/pyqt6/torque_profile_panel.py (612 LOC) IS grandfathered, with the weaker justification stated plainly in the baseline file rather than glossed: unlike the #4448 entries, this is a NEW file authored over the budget. ~503 of its 612 lines are a single QWidget subclass; the only clean seam is `TorquePolynomialDialog` (~33 lines), and extracting it still leaves 579. Going under 500 means carving up one Qt widget class, moving signal/slot wiring and layout construction across a module boundary for a line count. Tracked in #4503. Verified: ruff check and format clean, size budget 0 violations, zero deletions relative to main, zero gitlinks in the index. One pre-existing test failure is untouched and is NOT caused by this merge: tests/rate_of_closure/test_variation_ensemble_io_reader.py:: test_text_reader_normalizes_decoder_resource_errors fails identically with these changes stashed, so it predates them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The
testslane is the required check, and its most frequent failure is not a test — it's a download.Every run installed the Rust toolchain via rustup, fetched the crates.io registry into a fresh
per-workspace
CARGO_HOME, force-reinstalledmaturinfrom PyPI with--no-cache-dir, and recompiled thewheel, all before a single test ran.
Measured across the 2026-08-13 consolidation drive, that was the largest single source of red required
checks. Of ~20 job failures on the consolidation PRs, every one was a network step, and the Rust
toolchain / wheel build led the tally:
tools_corewheel buildInstall Dependencies(PyPI)Collect Changed Coverage Inputs(git fetch, one DNS failure)actions/checkout@v7Install detect-secrets(PyPI)#4446 and #4466 failed at the identical step (#16) on two different runners minutes apart. None of these
failures had anything to do with the change under test.
What this does
Most pull requests don't touch
rust_core/, so the wheel they need is byte-identical to the last one built.This caches the built wheel, keyed on everything that can change its contents or its ABI:
rust-toolchain.toml— pins the compiler, so a channel change invalidatesCargo.toml/Cargo.lock—tools_coreis a workspace memberrust_core/tools-core/Cargo.toml,pyproject.toml, andsrc/**/*.rsrunner.osand the Python version — the wheel is ABI-specificOn a hit, the toolchain install, the crate downloads and the compile are all skipped. On a miss it also
caches the cargo registry and target directory, mirroring the block already in
maturin-ai-backend.ymlsothe repo's two Rust builds cache the same way.
Three details worth review
directory, the entry can exist but be empty — and
actions/cachewill not overwrite a key that alreadymatched. So the build decision reads the filesystem rather than
cache-hit: a hit with no wheel fallsback to building, which is today's behaviour and never worse than it.
import tools_corecheck still gates everyrun. A poisoned cache fails loudly there rather than silently shipping an unusable extension module.
maturinis left unpinned. Dropping--force-reinstall --no-cache-diris what stops the guaranteedfresh download; every other maturin call site in this repo (
maturin-ai-backend.yml,publish-artifacts.yml,rust-quality-gate) uses a plainpip install maturin. Choosing a version is aseparate reviewed decision, not a drive-by change inside a caching fix.
Scope
Deliberately limited to the required
testslane.rust-quality-gatebuilds the wheel the same way andwould benefit identically, but it isn't a required check and keeping this diff reviewable matters more than
covering both at once.
This does not fix the underlying fleet problem — the
defaultrunner pool holds stalebusyleases sonothing dispatches there, which routes every required check onto the
bandwidth-drainingpool. That stillwants the runner services restarted. What this change does is remove the largest network surface from the
required lane so it stops failing for reasons unrelated to the code, and unlike a service restart it stays
fixed.
Related: #4479 (
.gitattributes/ CRLF), #4481 (delta CI selecting no tests for filesmainnevertouches), #4464 (fork-PR execution on self-hosted runners).