Skip to content

Commit a2cb041

Browse files
Stop 13 jobs rebuilding the same frontend on every commit (#9375)
* 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. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
1 parent ce568f7 commit a2cb041

4 files changed

Lines changed: 299 additions & 4 deletions

File tree

.github/actions/install-unsloth-local/action.yml

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,77 @@ runs:
9393
restore-keys: |
9494
uv-${{ runner.os }}-
9595
96+
# The frontend build is the other half of this step, and unlike the wheels it is
97+
# not a download at all. Measured over 13 distinct Linux jobs on main, using the
98+
# elapsed-second prefix below: a median 36s of a 74s install, 49% of it, and
99+
# 468s per commit spent producing byte-identical output. The uv cache above
100+
# already hits exactly (`Cache hit for: uv-Linux-<hash>`, one 31 kB straggler),
101+
# so what is left is compute, and the only way to stop paying it 13 times is to
102+
# not do it 13 times.
103+
#
104+
# studio/setup.sh decides whether to rebuild by mtime: it looks for anything
105+
# under frontend/ (maxdepth 1, minus bun.lock), frontend/src or frontend/public
106+
# NEWER than frontend/dist, and skips the build when it finds nothing. So the key
107+
# hashes exactly those three path groups. A hit then means the build inputs are
108+
# byte-identical, which is a strictly stronger statement than the mtime test it
109+
# rides on, and the restored dist is correct by construction rather than by luck.
110+
#
111+
# tests/studio/test_frontend_dist_cache.py pins the key against the paths setup.sh
112+
# actually reads, because the two drifting apart is silent: the cache would keep
113+
# hitting and start serving a dist built from inputs no longer in the key.
114+
#
115+
# bun.lock is IN the key even though the staleness check excludes it. The check
116+
# has to exclude it because the install regenerates it and it would self-trigger
117+
# every run; the cache has no such problem, and a lockfile change means different
118+
# dependencies and so a different bundle. That is deliberate and makes the cache
119+
# safer than the check.
120+
- name: Restore the built frontend
121+
id: fe-dist
122+
uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
123+
continue-on-error: true
124+
with:
125+
path: studio/frontend/dist
126+
# NO restore-keys, deliberately, and the opposite of the uv cache above. A
127+
# near-miss download cache still supplies most of the wheels, which is most of
128+
# the win. A near-miss dist is a bundle built from different source: wrong, not
129+
# partial. Only an exact match may be served.
130+
key: fe-dist-${{ runner.os }}-${{ hashFiles('studio/frontend/*', 'studio/frontend/src/**', 'studio/frontend/public/**') }}
131+
132+
- name: Make the restored frontend outrank its sources
133+
if: steps.fe-dist.outputs.cache-hit == 'true'
134+
shell: bash
135+
run: |
136+
# actions/cache restores through tar, which preserves the ORIGINAL mtimes. A
137+
# dist restored that way is older than the checkout that just wrote every
138+
# source file, so setup.sh's `find -newer dist` would find the whole tree
139+
# newer and rebuild anyway -- the cache would cost a download and save
140+
# nothing, while looking like it worked. Touching the directory is what makes
141+
# the hit count, and it is honest because the key already proved the inputs
142+
# are byte-identical.
143+
#
144+
# The directory only: the staleness check compares against `frontend/dist`
145+
# itself, not its contents.
146+
if [ ! -d studio/frontend/dist ]; then
147+
echo "::error::the frontend dist cache reported a hit but restored no directory"
148+
exit 1
149+
fi
150+
touch studio/frontend/dist
151+
echo "restored a prebuilt frontend; setup.sh will report it up to date"
152+
153+
- name: Refuse a frontend cache key that hashes nothing
154+
shell: bash
155+
env:
156+
FE_KEY: ${{ hashFiles('studio/frontend/*', 'studio/frontend/src/**', 'studio/frontend/public/**') }}
157+
run: |
158+
# hashFiles returns the empty string when a glob matches no file, which would
159+
# collapse every commit onto one key and serve an arbitrary dist. That is the
160+
# one way this cache can be actively wrong rather than merely useless, and it
161+
# would be invisible: the restore succeeds and the build is skipped.
162+
if [ -z "$FE_KEY" ]; then
163+
echo "::error::hashFiles matched no frontend sources, so the dist cache key is degenerate. The frontend layout moved; update the key and tests/studio/test_frontend_dist_cache.py together."
164+
exit 1
165+
fi
166+
96167
- name: Install Unsloth (--local, --no-torch)
97168
shell: bash
98169
env:
@@ -137,6 +208,17 @@ runs:
137208
# artifacts, and only the former are worth carrying between runners. Without it
138209
# this key grows without bound across 39 jobs and re-opens the eviction thrash
139210
# the GGUF caches were fixed for.
211+
# Main only, the rule every cache in this repo follows: a PR-scoped entry can only
212+
# be restored by re-runs of that same PR while still counting against the shared
213+
# budget, evicting the copy every PR can read.
214+
- name: Save the built frontend
215+
if: github.ref == 'refs/heads/main' && steps.fe-dist.outputs.cache-hit != 'true'
216+
uses: actions/cache/save@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
217+
continue-on-error: true
218+
with:
219+
path: studio/frontend/dist
220+
key: ${{ steps.fe-dist.outputs.cache-primary-key }}
221+
140222
- name: Save the uv download cache
141223
if: always() && github.ref == 'refs/heads/main' && steps.uv-cache.outputs.cache-hit != 'true'
142224
shell: bash

.github/workflows/workflow-trigger-lint.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,7 @@ jobs:
141141
tests/studio/test_compile_caches_are_per_worker.py \
142142
tests/studio/test_composer_rtl_bidi_attribute.py \
143143
tests/studio/test_frontend_dep_removal.py \
144+
tests/studio/test_frontend_dist_cache.py \
144145
tests/studio/test_gguf_smoke_phases_stay_independent.py \
145146
tests/studio/test_indicator_browsers_run_in_parallel.py \
146147
tests/studio/test_inference_smoke_http_diagnostics.py \
Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,188 @@
1+
# SPDX-License-Identifier: AGPL-3.0-only
2+
# Copyright 2026-present the Unsloth AI Inc. team. All rights reserved. See /studio/LICENSE.AGPL-3.0
3+
4+
"""The frontend dist cache and setup.sh's rebuild check must read the same inputs.
5+
6+
Measured over 13 distinct Linux jobs on main, the frontend build is a median 36s of a
7+
74s `install-unsloth-local`, 49% of it, and 468s per commit producing byte-identical
8+
output. The cache exists to stop paying that 13 times.
9+
10+
What makes it safe is not the cache action, it is the agreement between two places:
11+
12+
studio/setup.sh rebuilds when anything under frontend/ (maxdepth 1, minus
13+
bun.lock), frontend/src or frontend/public is NEWER than
14+
frontend/dist
15+
the action's cache key hashes exactly those three path groups
16+
17+
A hit therefore means the build inputs are byte-identical, which is strictly stronger
18+
than the mtime test it rides on. Break the agreement and nothing goes red: the cache
19+
keeps hitting and quietly starts serving a dist built from inputs the key no longer
20+
covers, and every job downstream tests a stale bundle that passes. That is the whole
21+
reason this file exists, and it is why it asserts against setup.sh's own source rather
22+
than a list written down here.
23+
24+
Three subtler failure modes are pinned too, each of which looks like success:
25+
26+
* `restore-keys` on this cache. A near-miss download cache still supplies most of the
27+
wheels; a near-miss dist is a bundle built from different source. Wrong, not partial.
28+
* A restore with no `touch`. actions/cache restores through tar, which preserves the
29+
original mtimes, so the restored dist is older than the checkout that just wrote
30+
every source file and setup.sh rebuilds anyway. The cache would cost a download,
31+
save nothing, and report a hit.
32+
* An empty `hashFiles`. It returns "" when a glob matches nothing, collapsing every
33+
commit onto one key and serving an arbitrary dist.
34+
"""
35+
36+
from __future__ import annotations
37+
38+
import re
39+
from pathlib import Path
40+
41+
import yaml
42+
43+
44+
REPO = Path(__file__).resolve().parents[2]
45+
ACTION = REPO / ".github" / "actions" / "install-unsloth-local" / "action.yml"
46+
SETUP_SH = REPO / "studio" / "setup.sh"
47+
48+
49+
def _steps() -> list[dict]:
50+
doc = yaml.safe_load(ACTION.read_text(encoding = "utf-8")) or {}
51+
return [s for s in (doc.get("runs") or {}).get("steps") or [] if isinstance(s, dict)]
52+
53+
54+
def _step(fragment: str) -> dict | None:
55+
for step in _steps():
56+
if fragment.lower() in str(step.get("name", "")).lower():
57+
return step
58+
return None
59+
60+
61+
def _restore_step() -> dict:
62+
step = _step("Restore the built frontend")
63+
assert step is not None, (
64+
"install-unsloth-local no longer restores a built frontend. If the cache was "
65+
"removed on purpose, delete this file; if it was renamed, retarget it."
66+
)
67+
return step
68+
69+
70+
def _key() -> str:
71+
return str((_restore_step().get("with") or {}).get("key", ""))
72+
73+
74+
def _key_globs() -> set[str]:
75+
"""The paths hashFiles() reads, normalised so a trailing /** does not matter."""
76+
inner = re.search(r"hashFiles\((.*?)\)", _key())
77+
assert inner, f"the dist cache key does not call hashFiles: {_key()!r}"
78+
return {
79+
g.strip().strip("'\"").removesuffix("/**").rstrip("/*").rstrip("/")
80+
for g in inner.group(1).split(",")
81+
}
82+
83+
84+
def _staleness_inputs() -> set[str]:
85+
"""The paths setup.sh's rebuild check compares against frontend/dist.
86+
87+
Read out of setup.sh rather than hardcoded: a list written here would agree with
88+
itself forever while setup.sh moved.
89+
"""
90+
text = SETUP_SH.read_text(encoding = "utf-8")
91+
block = re.search(
92+
r"Detect whether frontend needs building(.*?)end packaged/Tauri guard", text, re.S
93+
)
94+
assert block, "could not find the frontend staleness check in studio/setup.sh"
95+
body = block.group(1)
96+
found = set()
97+
for m in re.finditer(r'"\$SCRIPT_DIR/(frontend[^"]*)"', body):
98+
path = m.group(1)
99+
if path.endswith("/dist"):
100+
continue
101+
found.add("studio/" + path)
102+
return found
103+
104+
105+
def test_the_key_covers_every_path_the_rebuild_check_reads() -> None:
106+
missing = sorted(_staleness_inputs() - _key_globs())
107+
assert not missing, (
108+
f"studio/setup.sh decides to rebuild the frontend by looking at {missing}, and the "
109+
f"dist cache key does not hash them. A change to those files would not change the "
110+
f"key, so the cache would hit and serve a dist built from different source, and "
111+
f"nothing would go red. Key: {_key()!r}"
112+
)
113+
114+
115+
def test_the_key_does_not_hash_paths_the_rebuild_check_ignores() -> None:
116+
"""Not a style rule: an over-broad key silently destroys the hit rate.
117+
118+
bun.lock is the deliberate exception. setup.sh must exclude it because the install
119+
regenerates it and it would self-trigger every run; the cache has no such problem,
120+
and a lockfile change means different dependencies and so a different bundle. It is
121+
covered by the `studio/frontend/*` glob, which is why that glob is allowed to be
122+
broader than the check's maxdepth-1 scan rather than being narrowed to match it.
123+
"""
124+
extra = sorted(_key_globs() - _staleness_inputs())
125+
assert extra == [], (
126+
f"the dist cache key hashes {extra}, which setup.sh's rebuild check does not "
127+
f"read. Every unrelated edit to those paths would miss the cache for no reason. "
128+
f"If the extra path genuinely affects the built bundle, say so where the key is "
129+
f"defined and widen this test deliberately."
130+
)
131+
132+
133+
def test_the_dist_cache_has_no_restore_keys() -> None:
134+
with_ = _restore_step().get("with") or {}
135+
assert "restore-keys" not in with_, (
136+
"the frontend dist cache has restore-keys. A prefix hit would serve a bundle "
137+
"built from DIFFERENT source, which is wrong rather than partial. The uv "
138+
"download cache in the same action does want them, and that contrast is the "
139+
"point: a near-miss download still supplies most of the wheels."
140+
)
141+
142+
143+
def test_a_restored_dist_is_made_newer_than_the_checkout() -> None:
144+
step = _step("outrank its sources")
145+
assert step is not None, (
146+
"nothing touches the restored dist. actions/cache restores through tar, which "
147+
"preserves the original mtimes, so setup.sh's `find -newer dist` sees the whole "
148+
"freshly checked-out tree as newer and rebuilds anyway. The cache would report a "
149+
"hit, cost a download and save nothing."
150+
)
151+
body = str(step.get("run", ""))
152+
assert re.search(r"^\s*touch studio/frontend/dist\s*$", body, re.M), (
153+
f"the step meant to make the restored dist outrank its sources does not touch "
154+
f"studio/frontend/dist: {body!r}"
155+
)
156+
assert str(step.get("if", "")).strip() == "steps.fe-dist.outputs.cache-hit == 'true'", (
157+
"the touch must be gated on a cache hit, or a miss would touch a dist that was "
158+
"never restored and suppress the build that has to happen"
159+
)
160+
161+
162+
def test_a_degenerate_key_is_refused() -> None:
163+
step = _step("hashes nothing")
164+
assert step is not None, (
165+
'nothing refuses an empty hashFiles result. It returns "" when a glob matches '
166+
"no file, which collapses every commit onto one key and serves an arbitrary "
167+
"dist, with the restore succeeding and the build skipped."
168+
)
169+
assert "exit 1" in str(step.get("run", "")), "the degenerate-key check does not fail the job"
170+
171+
172+
def test_the_dist_cache_is_saved_on_main_only() -> None:
173+
step = _step("Save the built frontend")
174+
assert step is not None, "the dist cache is restored but never saved, so it can only ever miss"
175+
cond = str(step.get("if", ""))
176+
assert "refs/heads/main" in cond, (
177+
f"the dist cache is saved off main: {cond!r}. A PR-scoped entry can only be "
178+
f"restored by re-runs of that same PR while still counting against the shared "
179+
f"budget, evicting the copy every PR can read."
180+
)
181+
182+
183+
def test_the_guard_is_reading_real_files() -> None:
184+
"""Every assertion above passes vacuously if these two files stop being found."""
185+
assert ACTION.is_file(), ACTION
186+
assert SETUP_SH.is_file(), SETUP_SH
187+
assert len(_staleness_inputs()) >= 2, _staleness_inputs()
188+
assert len(_key_globs()) >= 2, _key_globs()

tests/studio/test_uv_cache_discipline.py

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,17 +55,41 @@ def _index_of(predicate) -> int:
5555
return -1
5656

5757

58-
def test_the_cache_holds_uvs_downloads_and_not_the_venv() -> None:
58+
# What this action is allowed to cache, and the argument for each. Anything else has
59+
# to be added here in a diff someone reads, with its own argument written down.
60+
#
61+
# .uv-cache uv's download cache. Content-addressed by URL and hash, so a
62+
# stale entry cannot serve wrong content; the worst it can do
63+
# is miss.
64+
# studio/frontend/dist the built frontend. NOT a download, so it does not get the
65+
# argument above and needs its own: it is a directory of static
66+
# assets with no absolute paths, no interpreter coupling and no
67+
# console scripts, which is exactly what makes a venv unsafe to
68+
# cache and this safe. Its key hashes the same inputs
69+
# studio/setup.sh checks before rebuilding, so a hit means the
70+
# build inputs are byte-identical rather than merely similar.
71+
# tests/studio/test_frontend_dist_cache.py holds that agreement
72+
# together and is where the reasoning lives.
73+
CACHEABLE_PATHS = (".uv-cache", "studio/frontend/dist")
74+
75+
76+
def test_the_cache_holds_downloads_and_build_output_but_never_the_venv() -> None:
5977
"""
6078
A venv cache would have to reason about the editable overlay, a moving
61-
unsloth-zoo pin, and absolute paths in console scripts. A download cache
62-
reasons about none of that, which is why this one is safe at all.
79+
unsloth-zoo pin, and absolute paths in console scripts. Neither of the two
80+
things this action caches reasons about any of that, which is why they are safe
81+
at all. The forbidden list below is the invariant with teeth and applies to
82+
every cache step regardless of which allowed path it uses.
6383
"""
6484
for step in _steps():
6585
if "cache" not in str(step.get("uses", "")):
6686
continue
6787
path = str((step.get("with") or {}).get("path", ""))
68-
assert ".uv-cache" in path, f"cache step points at {path!r}, not uv's download cache"
88+
assert any(allowed in path for allowed in CACHEABLE_PATHS), (
89+
f"cache step points at {path!r}, which is not one of the paths this action "
90+
f"is allowed to cache ({', '.join(CACHEABLE_PATHS)}). Add it to "
91+
f"CACHEABLE_PATHS with the argument for why restoring it cannot be wrong."
92+
)
6993
for forbidden in (".unsloth", "site-packages", "unsloth_studio", "venv"):
7094
assert forbidden not in path, (
7195
f"cache step points at {path!r}, which is an INSTALL, not a download "

0 commit comments

Comments
 (0)