Stop 13 jobs rebuilding the same frontend on every commit by danielhanchen · Pull Request #9375 · unslothai/unsloth · GitHub
Skip to content

Stop 13 jobs rebuilding the same frontend on every commit - #9375

Merged
danielhanchen merged 2 commits into
mainfrom
ci-cache-frontend-dist
Aug 20, 2026
Merged

Stop 13 jobs rebuilding the same frontend on every commit#9375
danielhanchen merged 2 commits into
mainfrom
ci-cache-frontend-dist

Conversation

@danielhanchen

Copy link
Copy Markdown
Member

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 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.

danielhanchen and others added 2 commits August 20, 2026 10:28
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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 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".

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge 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 👍 / 👎.

@danielhanchen
danielhanchen merged commit a2cb041 into main Aug 20, 2026
40 checks passed
@danielhanchen
danielhanchen deleted the ci-cache-frontend-dist branch August 20, 2026 11:22
danielhanchen added a commit that referenced this pull request Aug 20, 2026
…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>
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.

1 participant