Stop 13 jobs rebuilding the same frontend on every commit - #9375
Conversation
The uv download cache in this action works: it hits exactly (`Cache hit for: uv-Linux-<hash>`) with one 31 kB straggler still fetched. So what is left in `Install Unsloth (--local, --no-torch)` is not download, it is compute, and the elapsed-second prefix added in #9153 says where it goes: 2s venv 5s overlaying local repo (editable) 13s unsloth installed 16s node 20s bun installed 58s frontend built <- 38s in one phase 75s whisper.cpp prebuilt Measured across 13 distinct Linux jobs on main: the frontend build is a median 36s of a 74s install, 49% of it, and 468s per commit producing byte-identical output. The spread is 31 to 42s, so it is a deterministic compute cost rather than variance. The key, and why it is sound ------------------------------------------------------------------------ studio/setup.sh already decides whether to rebuild, by mtime: it looks for anything under frontend/ (maxdepth 1, minus bun.lock), frontend/src or frontend/public NEWER than frontend/dist, and skips the build when it finds nothing. The cache key hashes exactly those three path groups, so a hit means the build inputs are byte-identical. That is a strictly stronger statement than the mtime test it rides on, and it is what makes a restored dist correct by construction rather than by luck. bun.lock is IN the key even though the staleness check excludes it. The check has to exclude it because the install regenerates it and it would self-trigger every run; the cache has no such problem, and a lockfile change means different dependencies and so a different bundle. Deliberate, and it makes the cache safer than the check it rides on. Three ways this could have looked like it worked ------------------------------------------------------------------------ Each is handled, and each is pinned by tests/studio/test_frontend_dist_cache.py, because all three are silent. 1. restore-keys. The uv cache above wants them: a near-miss download still supplies most of the wheels. A near-miss dist is a bundle built from different source, which is wrong rather than partial, so this cache has none. 2. mtimes. actions/cache restores through tar, which preserves the ORIGINAL mtimes. A dist restored that way is older than the checkout that just wrote every source file, so setup.sh's `find -newer dist` would see the whole tree as newer and rebuild anyway: a download paid for, nothing saved, and a cache hit reported. One `touch` of the directory is what makes the hit count, and it is honest because the key already proved the inputs identical. 3. an empty hashFiles. It returns "" when a glob matches nothing, which collapses every commit onto one key and serves an arbitrary dist, with the restore succeeding and the build skipped. A step refuses that outright. The guard ------------------------------------------------------------------------ The failure that matters is not the cache breaking, it is the cache and setup.sh drifting apart: the key stops covering an input, the cache keeps hitting, and every job downstream tests a stale bundle that passes. So the guard reads setup.sh's own staleness block and asserts the key covers the paths found there, rather than comparing against a list written down in the test. Mutation-tested, each failing exactly one test: drop src from the key; add restore-keys; remove the touch; save off main; and add a directory to setup.sh's check without adding it to the key. A test I had to change rather than route around ------------------------------------------------------------------------ test_the_cache_holds_uvs_downloads_and_not_the_venv asserted every cache step in this action points at .uv-cache, and this is the second cache. Its argument is worth keeping: uv's cache is content-addressed, so a stale entry cannot serve wrong content, and that property is the whole justification. A built frontend does not get that argument and needs its own. It is a directory of static assets with no absolute paths, no interpreter coupling and no console scripts, which is precisely what makes a venv unsafe to cache and this safe. So the test now allows exactly two named paths, each with its reasoning recorded at the list, and keeps the forbidden-install-paths check applying to every cache step regardless. Verified it still has teeth: pointing the new cache at ~/.unsloth/studio/venv fails it. Verification ------------------------------------------------------------------------ 72 passed across test_uv_cache_discipline, test_frontend_dist_cache, test_workflow_guards_run_unfiltered and test_cache_budget_discipline. scripts/lint_workflow_triggers.py: OK across 41 workflow files. The action still parses; step order is restore, touch, key check, install, save. Expected effect: about 36s off each of 13 jobs per commit. Cache size is one built frontend per distinct source state, saved on main only, which is the rule every other cache here follows.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b8b7c2d5a
ℹ️ 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".
There was a problem hiding this comment.
Include the frontend build recipe in the cache key
When a PR changes studio/setup.sh's frontend build procedure without changing files under studio/frontend—for example, adding a generation phase or changing the build command—this key remains identical to main's cache key. The action therefore restores the old bundle and touches dist, causing the modified staleness logic to skip the build; after merge, the immutable main cache can keep every consumer on that stale bundle until a frontend file happens to change. Hash studio/setup.sh (or a dedicated build-recipe version) so changes to how the artifact is produced force a rebuild.
Useful? React with 👍 / 👎.
…ds (#9380) `studio/frontend/*` is RECURSIVE, which is the opposite of how it reads and the opposite of what #9375 assumed when it landed. @actions/glob runs with matchDirectories and implicitDescendants both true, so a bare `*` matches the subdirectory ENTRY and then expands it. Confirmed against the real library rather than inferred: pattern "fe/*" -> fe/public/p.txt fe/src/s.txt fe/tests/t.txt fe/top.txt pattern "fe/*.txt" -> fe/top.txt GitHub's documented non-recursive example uses an extension, so it never exercises the directory-matching path and does not apply to a bare `*`. The effect is that the key hashed all 457 files under frontend/tests and frontend/scripts, neither of which setup.sh's rebuild check reads. Editing any frontend TEST evicted a dist whose bundle is byte-identical. Two negations fix it, and they make the key mean what the comment beside it already claimed: exactly the three path groups setup.sh compares against dist. Not narrowed to an extension allowlist, deliberately. The 30 top-level files span 9 extensions including dotfiles, and a new top-level file type would then drop out of the key silently and let a STALE dist be served. The failure directions are not symmetric: an over-broad key wastes a rebuild, an under-broad key serves the wrong bundle. Negations keep the safe direction, because a subdirectory added later is not excluded and lands in the key. Why the existing guard did not catch this ------------------------------------------------------------------------ test_the_key_does_not_hash_paths_the_rebuild_check_ignores compares glob PREFIXES, so `studio/frontend/*` normalises to `studio/frontend` and the check can never see how far the glob descends. It passed while asserting something untrue, and its docstring called that glob a maxdepth-1 scan. Both are corrected, and the blind spot is now named where the test is. Two new guards, each derived from the tree and setup.sh rather than a list: - the exclusions must cover every frontend subdirectory the rebuild check never reads, so a directory added later surfaces as a decision instead of quietly costing hit rate; - no exclusion may cover a path the rebuild check DOES read, which is the direction that would serve a stale bundle while every downstream test still passed against a stale UI. Mutation-tested: dropping the tests exclusion fails the first; excluding src fails both. Verified: 69 passed across the three cache-discipline suites; the action parses with restore, touch, key check, install, save still in order; lint_workflow_triggers OK across 41 files. The key value changes, so the two existing fe-dist entries (35 and 36 MiB) are orphaned and replaced on the next main run. Co-authored-by: danielhanchen <unslothai@gmail.com>

Stop 13 jobs rebuilding the same frontend on every commit
The uv download cache in this action works: it hits exactly (
Cache hit for: uv-Linux-<hash>) with one 31 kB straggler still fetched. So what is left inInstall Unsloth (--local, --no-torch)is not download, it is compute, and theelapsed-second prefix added in #9153 says where it goes:
Measured across 13 distinct Linux jobs on main: the frontend build is a median
36s of a 74s install, 49% of it, and 468s per commit producing byte-identical
output. The spread is 31 to 42s, so it is a deterministic compute cost rather
than variance.
The key, and why it is sound
studio/setup.sh already decides whether to rebuild, by mtime: it looks for
anything under frontend/ (maxdepth 1, minus bun.lock), frontend/src or
frontend/public NEWER than frontend/dist, and skips the build when it finds
nothing. The cache key hashes exactly those three path groups, so a hit means the
build inputs are byte-identical. That is a strictly stronger statement than the
mtime test it rides on, and it is what makes a restored dist correct by
construction rather than by luck.
bun.lock is IN the key even though the staleness check excludes it. The check has
to exclude it because the install regenerates it and it would self-trigger every
run; the cache has no such problem, and a lockfile change means different
dependencies and so a different bundle. Deliberate, and it makes the cache safer
than the check it rides on.
Three ways this could have looked like it worked
Each is handled, and each is pinned by tests/studio/test_frontend_dist_cache.py,
because all three are silent.
restore-keys. The uv cache above wants them: a near-miss download still
supplies most of the wheels. A near-miss dist is a bundle built from different
source, which is wrong rather than partial, so this cache has none.
mtimes. actions/cache restores through tar, which preserves the ORIGINAL
mtimes. A dist restored that way is older than the checkout that just wrote
every source file, so setup.sh's
find -newer distwould see the whole tree asnewer and rebuild anyway: a download paid for, nothing saved, and a cache hit
reported. One
touchof the directory is what makes the hit count, and it ishonest because the key already proved the inputs identical.
an empty hashFiles. It returns "" when a glob matches nothing, which collapses
every commit onto one key and serves an arbitrary dist, with the restore
succeeding and the build skipped. A step refuses that outright.
The guard
The failure that matters is not the cache breaking, it is the cache and setup.sh
drifting apart: the key stops covering an input, the cache keeps hitting, and
every job downstream tests a stale bundle that passes. So the guard reads
setup.sh's own staleness block and asserts the key covers the paths found there,
rather than comparing against a list written down in the test.
Mutation-tested, each failing exactly one test: drop src from the key; add
restore-keys; remove the touch; save off main; and add a directory to setup.sh's
check without adding it to the key.
A test I had to change rather than route around
test_the_cache_holds_uvs_downloads_and_not_the_venv asserted every cache step in
this action points at .uv-cache, and this is the second cache. Its argument is
worth keeping: uv's cache is content-addressed, so a stale entry cannot serve
wrong content, and that property is the whole justification.
A built frontend does not get that argument and needs its own. It is a directory
of static assets with no absolute paths, no interpreter coupling and no console
scripts, which is precisely what makes a venv unsafe to cache and this safe. So
the test now allows exactly two named paths, each with its reasoning recorded at
the list, and keeps the forbidden-install-paths check applying to every cache step
regardless. Verified it still has teeth: pointing the new cache at
~/.unsloth/studio/venv fails it.
Verification
72 passed across test_uv_cache_discipline, test_frontend_dist_cache,
test_workflow_guards_run_unfiltered and test_cache_budget_discipline.
scripts/lint_workflow_triggers.py: OK across 41 workflow files.
The action still parses; step order is restore, touch, key check, install, save.
Expected effect: about 36s off each of 13 jobs per commit. Cache size is one
built frontend per distinct source state, saved on main only, which is the rule
every other cache here follows.