{{ message }}
Take the loaded-models indicator suite off the UI job's critical path - #9060
Merged
Conversation
Unsloth UI CI / Chat UI Tests has been timing out at its 30 minute limit on almost every open PR: 5 of the 6 currently open, at 30m09s to 30m24s. Nothing was failing. The job was simply longer than the limit, and a limit is a guillotine, so the steps after the cut never ran at all. Where it goes: "Cross-browser loaded-models indicator" is 853s of the 30 minutes, three browser engines at about 4.7 minutes each, each booting its own Unsloth. Everything downstream of it -- the other-engines update banner regression, both image staged-download regressions, the model-picker per-model-config suite and the IME / multilingual paste regression -- was reported "skipped" and has been running on no PR at all. It is now its own job rather than a step, so it runs beside the rest instead of inside their budget. That leaves ui-smoke around 16 minutes and the new job around 15, both with real margin, and the five skipped suites run again. The new job carries no HF cache and no GGUF: the suite stubs the four /status reads with page.route, so it needs no model, no GPU and no llama.cpp build. Moving the work rather than the limit is deliberate. The limit here already went 25 -> 30 for this exact symptom, with the comment on it still describing that round, so raising it again buys one more release of silence. Also tests/studio/test_playwright_suites_run_in_ci.py, because these drivers are standalone scripts that nothing collects: a suite runs only because a workflow step or a .github/scripts helper names it, and deleting that line leaves every job green. It fails if any tests/studio/playwright_*.py is named by no workflow and no CI script. Deleting either the new step or the driver call inside run-studio-indicator-browser.sh fails it. It also found two suites already in that state, exempted with a reason each rather than quietly ignored: playwright_tauri_python_tool_images.py needs the Tauri desktop shell no runner here builds, and playwright_train_pickers.py needs a dataset and model resolved through huggingface_hub while the UI workflows boot API-only. Both are worth wiring up, separately from this. test_playwright_suites_run_in_ci.py: 3 passed, and 2 failed under each mutation test_stt_model_search_locator_contract.py + test_composer_rtl_bidi_attribute.py: 34 passed
Reported on this PR and correct: reading every file under .github/scripts counts an orphaned helper as coverage, so a driver named only by a script nothing calls reads as covered while it runs nowhere, which is the regression this test exists to catch. The workflows now seed the text and a helper joins only once something already in it names the helper, repeated to a fixed point because one helper can call another. .github/actions is included the same way, since a workflow reaches a composite action by uses: and the action then calls the script. Verified by making the reference unreachable in all three UI workflows while leaving run-studio-indicator-browser.sh in the tree. The new version reports the driver as running nowhere; the scan-everything version reports it covered. That one helper is named by the Linux, Windows and Mac UI workflows, which is why removing it from one is correctly still covered. 3 passed
Reported on this PR and correct. Reading a workflow whole counts a driver named in on.pull_request.paths as an invocation, and studio-frontend-ci.yml names playwright_strip_ansi_smoke.py in both its trigger list and its step, so deleting the step alone left this guard green while the suite ran nowhere. Trigger paths say when CI runs, not what it runs. The scan now parses each workflow and reads only the executable fields: a job's uses, and each step's run, uses and with. Helper reachability is followed from that text as before. Verified by deleting only the run line in studio-frontend-ci.yml and leaving the trigger path: the guard now reports playwright_strip_ansi_smoke.py as running nowhere, where before it stayed green. 3 passed
Reported on this PR and correct. A composite action is used as `./.github/actions/<name>`, never as the action.yml inside it, so the reachability walk never opened one and a driver launched from inside an action read as an orphan. An action.yml now also matches on its containing directory. Pinned with a line from inside install-unsloth-local's own body rather than something a workflow could also contain, so the pin fails if the walk matches the uses: reference without opening the action. Dropping the directory match fails it. 3 passed
Reported on this PR and correct: the guard could not catch the regression it exists for. It ran only in studio-backend-ci.yml's Repo tests job, whose paths do not include .github/workflows/**, .github/scripts/** or .github/actions/**, so a PR that only deletes a Playwright invocation never collected it and stayed green until the post-merge run on main. It now runs in workflow-trigger-lint.yml instead, which carries no paths filter at all and says so in its own header comment, deliberately, for the same reason: a gate that only runs for some PRs does not gate workflow changes. Cost is the same shape as the lint beside it, filesystem reads in seconds, and that job already installs PyYAML, which is all this needs besides pytest. 3 passed, run bare from the repo root as that job runs it the workflow-trigger lint itself still passes: 42 workflow files scanned
Reported on this PR and correct: the repo-wide check cannot see this job disappear. run-studio-indicator-browser.sh is named by the Mac and Windows UI workflows too, so deleting all three calls from the Linux workflow leaves every guard green while the Chromium/Firefox/WebKit coverage this job exists for is gone. Asserted against the ui-indicator job itself rather than the file, so a step moved back into ui-smoke fails it too: that is the 30-minute limit this change moved the work out of. 4 passed, and deleting the three calls fails the new test by naming all three
for more information, see https://pre-commit.ci
Reported on this PR and correct, with a real instance: substring presence is not coverage. .github/scripts/kaggle_studio_ci/report.py names playwright_chat_ui.py inside a result description, and the workflows name drivers in comments and in trigger paths, so deleting every real invocation could leave this guard green on prose alone. Every driver and helper in this repo is run the same way, as an argument to an interpreter, so that is what is matched now: the name at the end of a path token being handed to python, python3, node, bash or sh. Composite actions keep the directory match, since a workflow reaches one through uses: rather than a command. Verified by removing every real invocation of playwright_chat_ui.py while leaving report.py's description in place. The substring is still present and the guard now reports the driver as running nowhere; before, it stayed green.
for more information, see https://pre-commit.ci
Reported on this PR and correct: commenting a command out is how one gets disabled, and the scan reads workflow run: bodies and helper scripts verbatim, so `# python tests/studio/x.py` matched the invocation shape through the whitespace before python. Comments are stripped before matching. Shell, YAML and Python all take # to end of line, and a # inside a string only ever appears in prose here, which is not an invocation either way. Verified by commenting out every real invocation of playwright_chat_ui.py: the guard now reports it as running nowhere, where before it stayed green. 4 passed
Comments and docstrings only. The code is byte-identical once docstrings are stripped, checked by comparing the parsed trees, and the tests still pass.
The cross-engine check tested raw step bodies for the substring, so prefixing all three invocations with # left it green while the job ran no browser suite, which is the regression it exists to prevent. It uses the same _uncommented() helper the repo-wide scan does, and a pin asserts the disabled form of one of those lines does not read as coverage.
Member
Author
|
@codex review |
danielhanchen
added a commit
that referenced
this pull request
Aug 17, 2026
…#9060) * Take the indicator suite off the UI job's critical path Unsloth UI CI / Chat UI Tests has been timing out at its 30 minute limit on almost every open PR: 5 of the 6 currently open, at 30m09s to 30m24s. Nothing was failing. The job was simply longer than the limit, and a limit is a guillotine, so the steps after the cut never ran at all. Where it goes: "Cross-browser loaded-models indicator" is 853s of the 30 minutes, three browser engines at about 4.7 minutes each, each booting its own Unsloth. Everything downstream of it -- the other-engines update banner regression, both image staged-download regressions, the model-picker per-model-config suite and the IME / multilingual paste regression -- was reported "skipped" and has been running on no PR at all. It is now its own job rather than a step, so it runs beside the rest instead of inside their budget. That leaves ui-smoke around 16 minutes and the new job around 15, both with real margin, and the five skipped suites run again. The new job carries no HF cache and no GGUF: the suite stubs the four /status reads with page.route, so it needs no model, no GPU and no llama.cpp build. Moving the work rather than the limit is deliberate. The limit here already went 25 -> 30 for this exact symptom, with the comment on it still describing that round, so raising it again buys one more release of silence. Also tests/studio/test_playwright_suites_run_in_ci.py, because these drivers are standalone scripts that nothing collects: a suite runs only because a workflow step or a .github/scripts helper names it, and deleting that line leaves every job green. It fails if any tests/studio/playwright_*.py is named by no workflow and no CI script. Deleting either the new step or the driver call inside run-studio-indicator-browser.sh fails it. It also found two suites already in that state, exempted with a reason each rather than quietly ignored: playwright_tauri_python_tool_images.py needs the Tauri desktop shell no runner here builds, and playwright_train_pickers.py needs a dataset and model resolved through huggingface_hub while the UI workflows boot API-only. Both are worth wiring up, separately from this. test_playwright_suites_run_in_ci.py: 3 passed, and 2 failed under each mutation test_stt_model_search_locator_contract.py + test_composer_rtl_bidi_attribute.py: 34 passed * Count only helper scripts a workflow can actually reach Reported on this PR and correct: reading every file under .github/scripts counts an orphaned helper as coverage, so a driver named only by a script nothing calls reads as covered while it runs nowhere, which is the regression this test exists to catch. The workflows now seed the text and a helper joins only once something already in it names the helper, repeated to a fixed point because one helper can call another. .github/actions is included the same way, since a workflow reaches a composite action by uses: and the action then calls the script. Verified by making the reference unreachable in all three UI workflows while leaving run-studio-indicator-browser.sh in the tree. The new version reports the driver as running nowhere; the scan-everything version reports it covered. That one helper is named by the Linux, Windows and Mac UI workflows, which is why removing it from one is correctly still covered. 3 passed * Count only the workflow fields that run something Reported on this PR and correct. Reading a workflow whole counts a driver named in on.pull_request.paths as an invocation, and studio-frontend-ci.yml names playwright_strip_ansi_smoke.py in both its trigger list and its step, so deleting the step alone left this guard green while the suite ran nowhere. Trigger paths say when CI runs, not what it runs. The scan now parses each workflow and reads only the executable fields: a job's uses, and each step's run, uses and with. Helper reachability is followed from that text as before. Verified by deleting only the run line in studio-frontend-ci.yml and leaving the trigger path: the guard now reports playwright_strip_ansi_smoke.py as running nowhere, where before it stayed green. 3 passed * Follow a composite action by the directory workflows reference it with Reported on this PR and correct. A composite action is used as `./.github/actions/<name>`, never as the action.yml inside it, so the reachability walk never opened one and a driver launched from inside an action read as an orphan. An action.yml now also matches on its containing directory. Pinned with a line from inside install-unsloth-local's own body rather than something a workflow could also contain, so the pin fails if the walk matches the uses: reference without opening the action. Dropping the directory match fails it. 3 passed * Run the coverage guard from the workflow that has no paths filter Reported on this PR and correct: the guard could not catch the regression it exists for. It ran only in studio-backend-ci.yml's Repo tests job, whose paths do not include .github/workflows/**, .github/scripts/** or .github/actions/**, so a PR that only deletes a Playwright invocation never collected it and stayed green until the post-merge run on main. It now runs in workflow-trigger-lint.yml instead, which carries no paths filter at all and says so in its own header comment, deliberately, for the same reason: a gate that only runs for some PRs does not gate workflow changes. Cost is the same shape as the lint beside it, filesystem reads in seconds, and that job already installs PyYAML, which is all this needs besides pytest. 3 passed, run bare from the repo root as that job runs it the workflow-trigger lint itself still passes: 42 workflow files scanned * Pin the three browser engines to the job that runs them Reported on this PR and correct: the repo-wide check cannot see this job disappear. run-studio-indicator-browser.sh is named by the Mac and Windows UI workflows too, so deleting all three calls from the Linux workflow leaves every guard green while the Chromium/Firefox/WebKit coverage this job exists for is gone. Asserted against the ui-indicator job itself rather than the file, so a step moved back into ui-smoke fails it too: that is the 30-minute limit this change moved the work out of. 4 passed, and deleting the three calls fails the new test by naming all three * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Count a driver as covered only where CI actually runs it Reported on this PR and correct, with a real instance: substring presence is not coverage. .github/scripts/kaggle_studio_ci/report.py names playwright_chat_ui.py inside a result description, and the workflows name drivers in comments and in trigger paths, so deleting every real invocation could leave this guard green on prose alone. Every driver and helper in this repo is run the same way, as an argument to an interpreter, so that is what is matched now: the name at the end of a path token being handed to python, python3, node, bash or sh. Composite actions keep the directory match, since a workflow reaches one through uses: rather than a command. Verified by removing every real invocation of playwright_chat_ui.py while leaving report.py's description in place. The substring is still present and the guard now reports the driver as running nowhere; before, it stayed green. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Stop a commented-out invocation counting as coverage Reported on this PR and correct: commenting a command out is how one gets disabled, and the scan reads workflow run: bodies and helper scripts verbatim, so `# python tests/studio/x.py` matched the invocation shape through the whitespace before python. Comments are stripped before matching. Shell, YAML and Python all take # to end of line, and a # inside a string only ever appears in prose here, which is not an invocation either way. Verified by commenting out every real invocation of playwright_chat_ui.py: the guard now reports it as running nowhere, where before it stayed green. 4 passed * Tighten the comments in the coverage guard Comments and docstrings only. The code is byte-identical once docstrings are stripped, checked by comparing the parsed trees, and the tests still pass. * Read the browser-engine guard through the comment stripper The cross-engine check tested raw step bodies for the substring, so prefixing all three invocations with # left it green while the job ran no browser suite, which is the regression it exists to prevent. It uses the same _uncommented() helper the repo-wide scan does, and a pin asserts the disabled form of one of those lines does not read as coverage. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
danielhanchen
added a commit
that referenced
this pull request
Aug 17, 2026
…#9027) * Stub unsloth in test_safetensors_reasoning_stream, and close the guard hole Three tests in this file fail on main if the file runs first, or on its own, against the dependency set Backend CI installs: ImportError: Unsloth: Please install unsloth_zoo via `pip install unsloth_zoo` Which PR, and which side is wrong: #5620 (2026-07-06) added this file, with no stub, from the start #8342 (2026-08-10) added the stub guard, which does not look here Neither is wrong about its own subject, and neither is reverted. #5620's tests are correct; they just relied on another file having stubbed unsloth first. #8342's guard exempts imports inside a function on purpose, because those are lazy and cannot fail COLLECTION, which is the disaster it was written for. The hole is that pytest.importorskip USED to make a lazy import safe and no longer does. Since pytest 8.2 it skips on ModuleNotFoundError only, and unsloth/_gpu_init.py raises a plain ImportError when unsloth_zoo is missing, so the call fails instead of skipping. #8342's exemption was sound when written and stopped being sound under a pytest it did not choose. Fix: install the same stub the sibling files install, at module scope. Stubbing rather than skipif keeps the coverage: the module under test is still the real core.inference.inference, and only unsloth is faked. A skipif would have made three tests silently stop running on the backend job. Robustification: a second guard beside #8342's, for modules reached through pytest.importorskip at any scope, with its own reason stated rather than folded into the collection guard. Verified against main's version of this file: 1 test module(s) reach a backend module that needs unsloth through pytest.importorskip without installing the stub ... ['test_safetensors_reasoning_stream.py'] It is pinned in both directions (an unstubbed sample is an offence, a stubbed one and an importorskip of numpy are not) so it cannot pass by matching nothing. I also swept the other five files that importorskip a core.* or unsloth* module without stubbing. Under the no-unsloth_zoo condition all five pass, so this file was the only live case: test_active_generations 84 passed test_bypass_permissions 117 passed test_diffusion_dataset_clips 22 passed test_mlx_inference_backend 80 passed, 10 skipped test_offline_gguf_cache_fallback 202 passed test_safetensors_reasoning_stream 3 FAILED, 10 passed Verified by hiding unsloth_zoo's metadata to reproduce the CI shape: before: 3 failed, 10 passed after: 13 passed * Drop the stubs once the module is built, so they do not outlive it The first version of this fix installed the unsloth stub and left it in sys.modules for the rest of the process. Staging CI caught it on all four interpreters: tests/test_audio_type_inconclusive.py:159: in test_the_stubs_do_not_outlive_this_module assert name not in sys.modules, name E AssertionError: unsloth That test exists precisely to forbid what I did. Every other file installs its stubs, performs its heavy import, then pops them, and a stub left behind is a cross-file leak: every other _stub_if_missing returns early when the name is already present, so its own bookkeeping never runs and its cleanup has nothing to undo. Traded this file's order dependency for a worse one, in other words. Now it follows the same shape as test_audio_type_inconclusive.py: stub, build core.inference.inference while the stubs are live, drop the stubs. The three tests reach the module through pytest.importorskip and get it from sys.modules, so the stubs only need to exist for that one import. It did not reproduce locally, which is why it reached staging: run the file alone and the victim's own _STUBBED list is empty, so its loop asserts nothing and passes vacuously. It takes xdist interleaving the two files on one worker to put the leak in front of that assertion. * Close two holes in the importorskip guard Both reported on the guard added by this PR, both confirmed by construction against the guard's own helpers. 1. The bare-name form was invisible. "from pytest import importorskip" then a bare importorskip("core.inference.inference") is a call on an ast.Name, not an attribute, so matching only pytest.importorskip returned no targets: bare-name form detected as target: [] Both call shapes are matched now. 2. A module-scope call was judged by the wrong boundary. The check scanned the whole module for a stub, so a file that calls importorskip at module scope and installs its stub BELOW that line read as safe: late-stub form judged stubbed (scanning whole module): True while the call runs during collection and the import has already raised. The boundary is now the call's own line at module scope, and the end of the module only inside a def, where the call really does run after the body. The pinning test covers all four shapes and both boundaries, so neither hole can reopen silently. The guard still names the file this PR fixes when run against main's version of it. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Judge import-time calls and dropped stubs correctly in the guard Two more gaps on the guard this PR adds, both verified against its own helpers before changing anything. 1. Definition-time calls were given the wrong boundary. _runtime_nodes stops at every def and class, so a call in a class body, a decorator, an annotation or a default expression was treated as lazy and got the end-of-module boundary: class body, stub below calls=[('core.inference.inference', 5)] judged_safe=True default arg, stub below calls=[('core.inference.inference', 5)] judged_safe=True All of those run while the module is being imported. Only a function BODY is deferred, so the scope walk now keeps class bodies and definition-time expressions and excludes just the body. No file in this tree does this today; all five class-level importorskip calls are inside methods, which really are lazy. Fixed anyway, because a guard that is wrong about when Python runs things is not much of a guard. 2. A lazy call was judged only on whether a stub was ever installed, not on whether it was still there. A module that installs the stubs, drops them again, and then reaches the heavy module lazily passes that check and still raises at test time. This PR's own file is safe only because it imports the target eagerly while the stubs are live, and nothing made that a requirement, so a copy that omitted the eager import would have read as safe. Now: a lazy call is an offence when the module drops its stubs and does not import the target itself. A module that keeps its stubs installed is fine lazily, which is why the removal is part of the condition rather than the eager import alone. The decision is one function used by both the guard and the test that pins it. My first version of this test reimplemented the logic instead, and a mutation that deleted the real rule left it green. scope walk narrowed back to _runtime_nodes -> 1 failed eager-import rule deleted -> 1 failed stub-removal condition dropped -> 1 failed * Judge a dropped stub at import time too, not only for lazy calls Reported on this PR and correct. The guard asked "were the stubs dropped again, and was the target never imported eagerly" only in the lazy branch, so a module that installs the stub, pops it, and then calls importorskip at module scope satisfied the install check and read as safe, while collection still dies: the stub is gone by the time the call runs and nothing put the target in sys.modules. Both questions now take the same boundary as the install check. _drops_stubs and _eagerly_imports stop at that line, so an import-time call is judged on the pops and imports ABOVE it and a lazy call on the whole module body, which is what the old lazy-only branch computed. Three cases pinned: stub-pop-call is an offence, stub-call-pop is not, and stub-import-pop-call is not. Mutating the branch back to the lazy-only form fails the new pin. 6 passed. * Four precision fixes to the importorskip guard All four reported on this PR, all four real, each pinned and each verified by reverting the fix and watching the pin fail. 1. An eager import only counts if the stubs were live for it. A file that probes the target in try/except ImportError before installing the stubs, then stubs, pops, and calls importorskip lazily, was read as safe. On the dependency-light matrix that probe fails and Python removes the half-initialised module, so the later call still reaches the real dependency. The check now asks _stubs_before for the statement's own line, so there is one answer to "are the stubs live" in this file rather than two that can drift. 2. A drop has to be OUR stubs being dropped. Any x.modules.pop counted, so an unrelated sys.modules.pop("routes.foo", None) in a properly stubbed file got that file reported as an offender, and a pop on any object with a .modules counted as well. The receiver must now be sys.modules and the statement must name the required stub the same way an install does, directly or through a module-level list it reads, which is how the real files spell it. 3. importorskip(modname = "...") is the documented signature (modname, minversion=None, reason=None, *, exc_type=None), and node.args is empty for it, so a file written that way walked straight past the guard. The keyword is read now. 4. exc_type=ImportError is pytest's own opt-in to skipping on a plain ImportError, which is the exact failure this guard is about, so such a call is safe unstubbed and flagging it was a false report. Those calls are excluded. exc_type=ModuleNotFoundError is the default and stays flagged. Confirmed against the pytest reference: exc_type arrived in 8.2, "must be ImportError or a subclass", and defaults to ModuleNotFoundError. 6 passed, and reverting each of the four in turn: 1 failed each time test_safetensors_reasoning_stream.py + the guard together: 19 passed * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Install peft on the backend job, and stop misreading unrelated cleanup as a stub drop Two items from this round, both reported here. 1. The eager import in test_safetensors_reasoning_stream.py was failing on the backend matrix and the handler was swallowing it, so the three importorskip tests SKIPPED rather than ran: 10 passed and 3 skipped, green either way. core/inference/inference.py imports peft at module scope, and peft is listed only in extras-no-deps.txt and no-torch-runtime.txt, neither of which this job installs. Confirmed both ways: the install log for a real matrix leg lists torchao and transformers and no peft, and hiding peft behind a meta_path finder locally reproduces exactly 10 passed and 3 skipped. Fixed by installing it rather than stubbing it. A stub does not work here: transformers probes importlib.util.find_spec("peft") during its own import, which raises ValueError on a stub whose __spec__ is None, so stubbing turns a silent skip into a collection error. Installing it also means these tests exercise the real import chain, which is the coverage the file is for. The swallow now records the error and a new test reads it back, so a module-scope dependency added to inference.py that this job does not install fails by name instead of quietly removing three tests. 2. _drops_stubs accumulated every module-level assignment target, so an unrelated cleanup list popped from sys.modules read as the unsloth stubs being dropped and got properly stubbed files reported as offenders. It now follows the link: a module-scope call naming the required stub identifies the helper, and whatever module-level name that helper appends to is the record. Two cases pinned, and the pin for the real shape now spells out the helper body, because that is what the link is followed through. test_safetensors_reasoning_stream.py + the guard: 20 passed with peft hidden: the new test FAILS by name instead of 3 silent skips restoring the old accumulation: 1 failed * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Install peft after the CPU torch wheel, not before it Reported on this PR and correct, and this was my mistake in the previous commit. peft declares `torch >=1.13.0` with no index constraint (checked against its wheel metadata), so placed above the CPU-index line it resolves torch from PyPI, which on Linux is the CUDA build. That version satisfies the >=2.4,<2.11 the next line asks for, so pip leaves it, and a nominally CPU-only job ends up carrying the CUDA distribution and its nvidia-* dependencies on all four legs: minutes of download against a limit this job was already cancelling at, and runner disk. Moved below the torch and transformers lines, where the pinned CPU torch already satisfies the dependency and nothing re-resolves. * Judge eager imports and helper calls by what actually runs at import Three items from this round, all reported here, all real. 1. An eager import under `if TYPE_CHECKING:` or `if False:` never runs, so it is not what left the target in sys.modules, and a file that stubs, "imports" there, then drops the stubs still raises. The walk pruned nothing and read it as safe. It now skips a branch whose test is a constant the interpreter will not take, or TYPE_CHECKING, and nothing beyond that is guessed at. 2. The opposite error in the same walk: a class body, a decorator, a default and an annotation all execute while the module is being imported, so an eager import written in one of them DOES cache the target. _runtime_nodes stopped at every def and class, so such a file was reported as an offender when it is safe. That is what _import_time_nodes was already built for, so the eager check uses it now, with the reachability pruning above on top. 3. A def is only deferred while nothing runs it. Where the MODULE BODY calls a helper, that helper executes during collection, so an importorskip inside it runs then too, and the end-of-module boundary let a stub installed BELOW the call site read as being in place. Those calls now take the line they are invoked from. Followed one level, which is the shape that occurs; a helper reached only through another helper keeps the deferred boundary rather than being guessed at. Four cases pinned: TYPE_CHECKING import is an offence, a class-body import is not, a helper called at import time takes the call site's line, and the same helper never called keeps the end-of-module boundary. Reverting each of the three fixes fails a pin. 6 passed, and 20 passed with test_safetensors_reasoning_stream.py * Resolve importorskip aliases, and prune the else of a constant-true test Both reported on this PR, both real, both pinned and mutation-checked. 1. `from pytest import importorskip as ios` then `ios(...)` is a valid call, and matching the callee against the literal string missed it, so an unstubbed module written that way walked past the guard. Bare names bound to it are collected now, through the import and through a plain rebinding of the attribute form. `from mymod import importorskip` is not one of them and is pinned as such. 2. The other half of the unreachable-branch hole I closed last round: pruning the body of `if False:` while still descending into the `else:` of `if True:` left an import that never runs counting as the one that cached the target. The traversal now takes only the branch the interpreter takes, for either constant. Both directions pinned. 6 passed, and reverting each fix fails its pin * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Follow import-time helper calls all the way down Reported on this PR and correct, and my "conservative" one-level stop was conservative in the wrong direction. Stopping hands the inner helper the end-of-module boundary, which is the LENIENT answer: a stub installed anywhere in the file then reads as in time, while collection has already run the inner import and failed. Reachability now propagates through the module-level call graph to a fixed point, and a helper reached only through another helper inherits the outer call site's line, which is when it actually runs. Two cases pinned: the module calls the outer helper and the inner one holds the importorskip, which is an offence at the outer call's line; and the same chain with nothing calling it at import time, which keeps the deferred boundary. Removing the closure fails the first. 6 passed * Propagate only through the calls the helper actually makes Reported on this PR and correct, and this one fails a SAFE file rather than passing an unsafe one. ast.walk visits a call under `if False:` and a call inside a nested def that nothing invokes, and handing those the outer helper's import-time boundary judged an importorskip as running before a later stub when Python never executes that path during collection. It now walks the helper's body under the same reachability rule the module body uses, so the two cannot answer differently. Two cases pinned, both safe: an inner call under a constant-false test, and one inside a nested def that is returned rather than called. Reverting to ast.walk fails them. 6 passed * Do not report a call the interpreter never reaches Reported on this PR and correct, and it is the same false-positive direction as the last one: an importorskip under `if False:` or `if TYPE_CHECKING:` was collected as a module-scope call, so the guard failed a file that only type-checks the import or deliberately disables it. The helper seeding had the same gap for a helper invoked only inside such a branch. Both traversals are reachability-aware now. The call walk prunes constant branches wherever they appear, inside a function body as well as at module scope, since a call under `if False:` never runs there either. Three cases pinned, all safe: a TYPE_CHECKING call, an `if False:` call, and a helper called only from a disabled branch. Reverting either traversal fails them. 6 passed, and 20 passed with test_safetensors_reasoning_stream.py * Attribute a call to the innermost function that holds it Reported on this PR and correct, and the same false-positive direction as the last two. ast.walk descends into a nested def, so an importorskip written there was attributed to the enclosing module-level function and handed that function's import-time boundary. A nested body cannot run until something calls it, by which time a stub installed below the outer call is in place, so the guard rejected a file that works. Nested bodies are deferred now, and a call inside one falls through to the end-of-module boundary, which is what deferred means. Pinned: an importorskip inside a nested def that the outer helper returns rather than calls, with the stub after, is safe. Restoring ast.walk fails it. 6 passed, and 20 passed with test_safetensors_reasoning_stream.py * Stub the heavy imports in the peft-gated TTS test Reported on this PR, real, and a consequence of installing peft in the previous commit. test_audio_tts_cancellation.py gates on pytest.importorskip("peft") and then imports core.inference.inference in the test body without stubbing anything. That gate used to skip, so the import never ran. With peft installed it opens, and the import works only because collecting test_safetensors_reasoning_stream.py has already cached that module: module-level code runs during COLLECTION, before any test body, so the whole suite hides it. Running the file on its own does not. That is the same order dependence this PR exists to remove, reintroduced by the fix, which makes it worth more than the P2 it came in as. The file now stubs unsloth and trl, imports core.inference.inference while they are live, and drops them again, exactly as its siblings do. The peft gate stays, so a machine without peft still skips rather than failing. Verified: with peft hidden the file skips cleanly on its own (17 passed, 1 skipped), and 59 passed across it plus test_audio_type_inconclusive.py, test_safetensors_reasoning_stream.py and the guard. Locally the eager import is blocked only by a torch/torchao ScalingType mismatch inside peft, which is present on main too and has nothing to do with unsloth; with the stubs installed that is the sole remaining blocker, checked directly. Worth flagging separately: seven more files are in the same class, importing a heavy backend module lazily while never stubbing -- test_chat_eos_template_refresh.py, test_chat_template_continuation.py, test_control_markup_neutralize_7066.py, test_generation_timing.py, test_nudge_tool_calls_wiring.py, test_online_tokenization_wiring.py and test_training_preflight.py. All are masked by the same collection-time caching. They are latent rather than failing, and fixing eight files belongs in its own change rather than inflating this one. * Widen the stub guard to module-scope with/if blocks and helper-held stubs _first_heavy_import_line only read the direct children of the module body, so a heavy import inside a module-scope with or if was invisible to the guard: a file written as 'with something(): from core.training.trainer import X' without stubs would take collection down unseen. It now uses the import-time traversal, with try/except ImportError exempt since that is a deliberate guard. That widening needs the matching read on the other side. Two files hold their stubs with 'with _stubbed():' and import the heavy module inside that block, so the installing code is one call away from module scope and the statement spells neither the module name nor sys.modules. _stubs_before now reads through a call to a module-level helper, the same way the stub-record check already does, and keeps asking what the helper DOES rather than merely what it names. Pins added for both halves plus the helper that names the module without installing anything; each is mutation-checked. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Take the backend CI peft version from the requirements pin An unconstrained 'pip install peft' takes the newest release (0.20.0 on the last staging run), while extras-no-deps.txt pins peft==0.18.1 deliberately, because 0.19.0 breaks Unsloth's export subprocess. That left this job exercising the newly enabled inference tests against a peft that production never ships and no other job installs. The version is read out of extras-no-deps.txt rather than written here again, so a change to the pin cannot leave this workflow behind. peft 0.18.1 declares torch>=1.13.0, transformers unbounded and python>=3.10, so it holds across the 3.10 to 3.13 matrix and does not disturb the CPU torch installed above it. * Make both sides of the stub guard reachability and order aware Two follow-ons to widening the search into module-scope compound statements. The widened search used the plain import-time traversal, which descends into both branches of every conditional, so an import under if TYPE_CHECKING: or if False: read as an import-time dependency. A file with a legitimate type-only import would have failed this guard until someone added a stub it does not need. It uses the reachability-aware traversal now. And widening one side without narrowing the other read the whole enclosing statement as preceding the import, so if True: import core.inference.inference _stub_if_missing("unsloth", ()) counted as stubbed while Python attempts that import first and collection dies. Only the part of a statement that runs before the import line is read now. The stub side gets the same reachability treatment, from the other direction: a stub installed under if False: installs nothing, so counting it would report a file safe that still raises. Three pins added, each mutation-checked. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Follow the import's own branch, and match except clauses by what they catch Line order alone merges branches that exclude each other: if enabled: _stub_if_missing("unsloth", ()) else: import core.inference.inference puts the stub above the import while the two can never both run, and the guard called that file stubbed. What is walked now is the import's own chain: at each level only the block CONTAINING the import is descended into, and only the statements above it there. Those did run, because the import running means its branch was taken. A stub that merely might have run does not count either. That last rule needs one exemption, and the tree already had the case: test_training_progress_callback.py installs its stubs under 'if not _TRAINER_PRE_IMPORTED', where the flag is '"core.training.trainer" in sys.modules'. Skipping the stubs on the other branch is not an omission, because the import resolves out of sys.modules there and never reaches the real dependency. A test that asks sys.modules whether the target is already imported keeps its branch, read through a module-level flag as well as directly, since that is how it is written. Separately, the ImportError exemption matched substrings, so 'except MyImportError:' and 'except ExceptionGroup:' both exempted a try that catches neither. Handler types are compared as whole names now, including tuple members and the last component of a dotted name. ModuleNotFoundError is deliberately not in the set: it is a subclass, so it catches strictly less. Ten pins added across the four behaviours, each mutation-checked. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Take the loaded-models indicator suite off the UI job's critical path (#9060) * Take the indicator suite off the UI job's critical path Unsloth UI CI / Chat UI Tests has been timing out at its 30 minute limit on almost every open PR: 5 of the 6 currently open, at 30m09s to 30m24s. Nothing was failing. The job was simply longer than the limit, and a limit is a guillotine, so the steps after the cut never ran at all. Where it goes: "Cross-browser loaded-models indicator" is 853s of the 30 minutes, three browser engines at about 4.7 minutes each, each booting its own Unsloth. Everything downstream of it -- the other-engines update banner regression, both image staged-download regressions, the model-picker per-model-config suite and the IME / multilingual paste regression -- was reported "skipped" and has been running on no PR at all. It is now its own job rather than a step, so it runs beside the rest instead of inside their budget. That leaves ui-smoke around 16 minutes and the new job around 15, both with real margin, and the five skipped suites run again. The new job carries no HF cache and no GGUF: the suite stubs the four /status reads with page.route, so it needs no model, no GPU and no llama.cpp build. Moving the work rather than the limit is deliberate. The limit here already went 25 -> 30 for this exact symptom, with the comment on it still describing that round, so raising it again buys one more release of silence. Also tests/studio/test_playwright_suites_run_in_ci.py, because these drivers are standalone scripts that nothing collects: a suite runs only because a workflow step or a .github/scripts helper names it, and deleting that line leaves every job green. It fails if any tests/studio/playwright_*.py is named by no workflow and no CI script. Deleting either the new step or the driver call inside run-studio-indicator-browser.sh fails it. It also found two suites already in that state, exempted with a reason each rather than quietly ignored: playwright_tauri_python_tool_images.py needs the Tauri desktop shell no runner here builds, and playwright_train_pickers.py needs a dataset and model resolved through huggingface_hub while the UI workflows boot API-only. Both are worth wiring up, separately from this. test_playwright_suites_run_in_ci.py: 3 passed, and 2 failed under each mutation test_stt_model_search_locator_contract.py + test_composer_rtl_bidi_attribute.py: 34 passed * Count only helper scripts a workflow can actually reach Reported on this PR and correct: reading every file under .github/scripts counts an orphaned helper as coverage, so a driver named only by a script nothing calls reads as covered while it runs nowhere, which is the regression this test exists to catch. The workflows now seed the text and a helper joins only once something already in it names the helper, repeated to a fixed point because one helper can call another. .github/actions is included the same way, since a workflow reaches a composite action by uses: and the action then calls the script. Verified by making the reference unreachable in all three UI workflows while leaving run-studio-indicator-browser.sh in the tree. The new version reports the driver as running nowhere; the scan-everything version reports it covered. That one helper is named by the Linux, Windows and Mac UI workflows, which is why removing it from one is correctly still covered. 3 passed * Count only the workflow fields that run something Reported on this PR and correct. Reading a workflow whole counts a driver named in on.pull_request.paths as an invocation, and studio-frontend-ci.yml names playwright_strip_ansi_smoke.py in both its trigger list and its step, so deleting the step alone left this guard green while the suite ran nowhere. Trigger paths say when CI runs, not what it runs. The scan now parses each workflow and reads only the executable fields: a job's uses, and each step's run, uses and with. Helper reachability is followed from that text as before. Verified by deleting only the run line in studio-frontend-ci.yml and leaving the trigger path: the guard now reports playwright_strip_ansi_smoke.py as running nowhere, where before it stayed green. 3 passed * Follow a composite action by the directory workflows reference it with Reported on this PR and correct. A composite action is used as `./.github/actions/<name>`, never as the action.yml inside it, so the reachability walk never opened one and a driver launched from inside an action read as an orphan. An action.yml now also matches on its containing directory. Pinned with a line from inside install-unsloth-local's own body rather than something a workflow could also contain, so the pin fails if the walk matches the uses: reference without opening the action. Dropping the directory match fails it. 3 passed * Run the coverage guard from the workflow that has no paths filter Reported on this PR and correct: the guard could not catch the regression it exists for. It ran only in studio-backend-ci.yml's Repo tests job, whose paths do not include .github/workflows/**, .github/scripts/** or .github/actions/**, so a PR that only deletes a Playwright invocation never collected it and stayed green until the post-merge run on main. It now runs in workflow-trigger-lint.yml instead, which carries no paths filter at all and says so in its own header comment, deliberately, for the same reason: a gate that only runs for some PRs does not gate workflow changes. Cost is the same shape as the lint beside it, filesystem reads in seconds, and that job already installs PyYAML, which is all this needs besides pytest. 3 passed, run bare from the repo root as that job runs it the workflow-trigger lint itself still passes: 42 workflow files scanned * Pin the three browser engines to the job that runs them Reported on this PR and correct: the repo-wide check cannot see this job disappear. run-studio-indicator-browser.sh is named by the Mac and Windows UI workflows too, so deleting all three calls from the Linux workflow leaves every guard green while the Chromium/Firefox/WebKit coverage this job exists for is gone. Asserted against the ui-indicator job itself rather than the file, so a step moved back into ui-smoke fails it too: that is the 30-minute limit this change moved the work out of. 4 passed, and deleting the three calls fails the new test by naming all three * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Count a driver as covered only where CI actually runs it Reported on this PR and correct, with a real instance: substring presence is not coverage. .github/scripts/kaggle_studio_ci/report.py names playwright_chat_ui.py inside a result description, and the workflows name drivers in comments and in trigger paths, so deleting every real invocation could leave this guard green on prose alone. Every driver and helper in this repo is run the same way, as an argument to an interpreter, so that is what is matched now: the name at the end of a path token being handed to python, python3, node, bash or sh. Composite actions keep the directory match, since a workflow reaches one through uses: rather than a command. Verified by removing every real invocation of playwright_chat_ui.py while leaving report.py's description in place. The substring is still present and the guard now reports the driver as running nowhere; before, it stayed green. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Stop a commented-out invocation counting as coverage Reported on this PR and correct: commenting a command out is how one gets disabled, and the scan reads workflow run: bodies and helper scripts verbatim, so `# python tests/studio/x.py` matched the invocation shape through the whitespace before python. Comments are stripped before matching. Shell, YAML and Python all take # to end of line, and a # inside a string only ever appears in prose here, which is not an invocation either way. Verified by commenting out every real invocation of playwright_chat_ui.py: the guard now reports it as running nowhere, where before it stayed green. 4 passed * Tighten the comments in the coverage guard Comments and docstrings only. The code is byte-identical once docstrings are stripped, checked by comparing the parsed trees, and the tests still pass. * Read the browser-engine guard through the comment stripper The cross-engine check tested raw step bodies for the substring, so prefixing all three invocations with # left it green while the job ran no browser suite, which is the regression it exists to prevent. It uses the same _uncommented() helper the repo-wide scan does, and a pin asserts the disabled form of one of those lines does not read as coverage. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * Keep the killed-by-signal exit status when release() fails (#9072) * Keep the killed-by-signal exit status when release() fails A cancelled launcher reported success. The signal handler deletes the kernels and then re-raises the signal so the status still reads killed, but the delete was outside any try: a raise there propagated out of the handler into whatever the main thread was doing, main()'s except BaseException caught it, and finish() called release() a second time. When that second call worked, which is what a transient OSError out of a subprocess spawn on a loaded runner looks like, main RETURNED 0 and the job read as completed. Caught on a contended runner rather than by inspection, and reproduced by failing the first delete only: returncode 0 instead of -SIGTERM. The delete is now best effort, retried once, and the death by signal happens either way. If both attempts fail the slugs stay in the registry and the next launcher's orphan sweep reclaims them, which is the same path a kill -9 already takes. The subprocess waits in the suite move from 30s to a named budget of 120s. They guard against a launcher that never dies, not against latency, and the repo suite now runs four of these at once on a four-core runner. * Make the stall outlast the death budget, and assert the signal did the killing Raising the wait to 120s put it past the 60s stall the launcher sits in, which defangs the tests it was meant to make robust: a handler that swallows its signal leaves the process asleep and then resuming, so it wakes, runs finish(), deletes the kernels through the ordinary path and exits INSIDE the wait. The deletion assertions are satisfied by that, so those tests would have passed on a launcher that ignored the signal entirely. The stall is now a named 900s against the named 120s budget, and a test asserts the relationship rather than leaving it to whoever next tunes one of the two numbers. The deletion test also asserts the exit status now. Without it, the deletion is satisfied by finish() doing its usual work, so nothing there said the signal was what caused it. Verified by making the handler return instead of re-raising: with the stall at 900s that fails all four signal tests, and with it at 60s the relationship test fails as well. * Tighten the comments added here * Read a stub helper only as far as it has run Reading the whole helper body accepted a sys.modules write below the yield of a context manager. That code runs on the way OUT of the with block, after the import inside it has already been attempted, so it stubs nothing for that import and the file still dies at collection while the guard stays green. The scan now stops at the helper's first yield, and drops unreachable and merely optional branches the way the module-scope side already does. The yield has to be the helper's own: one belonging to a generator defined inside it says nothing about when the helper suspends, and taking it cut the scan off above the install the real files do perform. Three pins, each mutation-checked. * Count only the finally of a try as certain A try body whose handler swallows the exception is exactly a block that may stop part way: an optional import fails, the handler catches it, the stub call below it never runs, and the heavy import after the try does. Counting the whole body reported that file stubbed. Only the finally counts now, which runs on both paths. The else and the handlers are conditional by construction and were already excluded. No test module in the tree installs its stubs inside a try body, so nothing legitimate changes. Two pins, both mutation-checked. * Read the polarity of the already-imported guard, and reach nested helpers Three holes, all about when code runs rather than what it says. The already-imported exemption accepted any branch whose test mentioned sys.modules, either way up. Written as 'if _PRE:' the stubs are installed only when they are not needed and skipped when they are, and the import then fails at collection with the guard green. The polarity is read now, through a negation and through a module-level flag, and only the branch that runs when the module is ABSENT counts, since that is the one whose stubs the import needs. A while-else was treated as certain. It is skipped when the loop leaves through break, so it is not. And nested defs were left out of the function map entirely. Deferring a nested body is right while nothing calls it, but an outer helper the module body calls, which defines and calls an inner one, runs that inner body during collection. The call inside it was handed the end-of-module boundary, the lenient answer, so a stub installed after the outer call read as being in time while the inner import had already run. Nested defs are in the map now and inherit the boundary of the call that reaches them; one nothing calls at import is still deferred. Seven pins across the three, each mutation-checked. * Judge handlers, helper exits and stub calls by what they do Three more exemptions that were granted on shape rather than effect. A handler naming ImportError does not necessarily absorb it: 'except ImportError: raise', and one raising a replacement, both take collection down while the try body read as guarded, which hid the import from the guard entirely. Any raise the handler can reach disqualifies it now. A module-level pytest.skip is a call rather than a raise, so the skip idiom these files use stays exempt. A helper that can return before installing has not installed anything, so the scan stops at an exit rather than reading on to the call below it. Two exits are benign and both are already in the tree: a return under 'name in sys.modules', and one under the importlib probe inside an absorbing try. On each of those paths the module is AVAILABLE, so skipping the stub is the point of the branch. Without that distinction five correct files became offenders, which is how the shape of the exemption was found. And a call was treated as installing a stub because its name contained 'stub', which counted _remove_stub and _validate_stub. Where the module defines the callee, what it does decides. The name stays as the fallback for a helper imported from elsewhere, since a name defined nowhere would raise NameError at import and never reach collection. Nine pins, four mutation-checked behaviours. * Honour handler dispatch order, and stop at deferred bodies and dead code Python dispatches an exception to the FIRST handler that matches it, so 'except Exception: raise' followed by 'except ImportError: pass' still propagates. Asking whether ANY handler absorbs found the second one and exempted a try that does not guard the import. Only the first handler that would catch it is asked now. The re-raise check also descended into a def the handler merely DEFINES. That body does not run while the exception is being handled, so a properly guarded file was reported as an offender. The traversal stops at deferred bodies now. And the call-graph closure walked past an unconditional exit, handing a helper named below one the outer call's import-time boundary although it never ran, which rejected a stub installed below that call. It stops at such an exit now, checked AFTER the statement since 'return _inner()' runs its own expression, and only for an unconditional one: the path that skips a conditional exit still reaches the calls below it. Five pins, three mutation-checked behaviours. * Keep every definition of a helper name, and stop reachability at dead code Two enclosing functions can each define a helper of the same name. Keeping only the first dropped the second from the call graph, so its importorskip took the end-of-module boundary and a stub installed after the enclosing call read as being in time. All definitions of a name are kept now. Which one a given call names cannot be told apart here, so all of them take the boundary, which is the strict answer. And the general reachability walk did not stop at an unconditional exit, so an importorskip written after a bare return or raise was reported as an unstubbed offence. Python cannot execute it, so that is a file failing CI over dead code. The walk now ends a statement list at a return, raise, break or continue, which the import-time traversal beside it already did. Four pins, both mutation-checked. * Judge every heavy import, not just the first Separate branches can each import a heavy module, and only one of them needs to be stubbed for the first to look fine. A file that stubs before the import in the if and forgets the one in the else dies at collection whenever the else runs, while reducing the module to a single line reported it safe. Every import-time heavy import is collected now and each is judged against the stubs that precede it on its own path. Any single unstubbed one is an offence, whatever the others do. Two pins, mutation-checked. * Require the availability guard to name the module, and drop async installers The already-imported exemption fired on any sys.modules membership test, so 'if "pytest" not in sys.modules:' decided whether the stubs under it counted. An unrelated cached package says nothing about unsloth, and the branch is skipped exactly when that package is present. The key now has to be the required stub or a heavy backend module. A key this file cannot read gets that latitude only inside a stub helper deciding about its own argument, which is the idiom _stub_if_missing opens with, and never at module scope. And calling an async def builds a coroutine and runs none of its body, so reading through such a call credited the module with stubs it never installed. Only synchronous helpers are read through now. Six pins, three mutation-checked behaviours. * Drop the review bookkeeping from the comments Thirty-three comments ended in 'Reported on this PR', which says nothing to anyone reading the file afterwards: once merged there is no this PR. The reasoning each comment records is what matters and is kept. * Name the file rather than the pull request --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
shimmyshimmer
added a commit
that referenced
this pull request
Aug 25, 2026
The vite install in the banner shard exists for playwright_overlay_rail.py and nothing else, so with the harness gone it is dead. Nothing left in studio-ui-smoke.yml touches npm, npx, node_modules or studio/frontend, so on a frontend-dist cache hit it would run a clean `npm ci` for no consumer. logs/playwright-overlay-rail goes with it: it was only ever written by the two steps this branch removed. Job-level setup-node stays. It predates the harness (#9060) and the installer needs node to build the frontend on a cache miss.
shimmyshimmer
added a commit
that referenced
this pull request
Aug 25, 2026
) * Studio: put the notification rail back in its bottom-right corner The rail was anchored with `fixed bottom-4 right-4` until #8082, which added the loaded models indicator and, with it, a frame store the rail read: the Live monitor, the API monitor panel and the chat composer publish their boxes, and `useStackGeometry` wrote `bottom` and `maxHeight` as inline styles that dodged them. Every input to that placement moves on its own. The composer grows with its input and goes from centred to docked with no resize event, a download row arrives, release notes expand, a banner is dismissed. Each change re-ran the measurement, and when the room fell short the rail lifted over the obstacle at `viewportHeight - frame.top + STACK_GAP`. On the welcome chat that is the cards in the middle of the window; against a maximised monitor the lift clamped at MIN_STACK_ROOM and parked them at the top. It reads as random because the trigger is measurement timing. The store also had to probe its own output, dropping `max-height` to read `scrollHeight` and restoring it with transitions suppressed, because the placement was feeding on the box it had just sized. It went from 184 lines to 701 over nine follow-up PRs, all of them editing the same arithmetic. So this restores the anchor rather than adding another special case. The rail is `fixed bottom-4 right-4` with the CSS cap again, on the named layer rather than the old hard-coded z-index. The frame store keeps its map of boxes, which api-monitor/panel-placement still needs to open the API panel clear of the monitor and the composer, and loses the dodge arithmetic, the measurement loop and the `coverable` licence. The composer still publishes for that panel's sake. The indicator itself is untouched. Known trade-off, and the pre-#8082 behaviour: in a short window a card can sit over the composer's Send button rather than the rail dodging it. The indicator is off by default, draggable and dismissible. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Studio: scroll the rail at its cap instead of spilling cards off screen Review catch, and it reproduces. The cap alone does not contain anything: the cards that cannot shrink lay out past the rail's bottom edge, which is pinned 16px off the floor, so the overflow goes under the bottom of the window with no way to bring it back. At 320x480 with --ui-font-scale 1.25, three cards up: the llama.cpp banner is in flow with its bottom at 497 against a 480px viewport, and the card after it is squeezed to 0px at 505. scrollHeight 501 against clientHeight 448, and overflow-y visible. The Update button is below the fold for good. Earlier sizes missed it because the download panel is the one min-h-0 card in the rail and absorbed the squeeze on its own. Raising the type size puts the two banner floors over the cap and the slack runs out. So the rail scrolls again, in CSS only. No measurement, no placement: the offset and the cap stay where they were. The gutter comes back with it, since overflow-y forces overflow-x and the card shadows clip without it, and stays horizontal so the bottom card keeps sitting on bottom-4. The rail stays click-through. Verified that the fold is still reachable: elementFromPoint over a card returns a node inside it, so the card is the hit target despite the rail being pointer-events-none, and the rail is that node's nearest scrollable ancestor, which is the chain a wheel scrolls. Focus works too: the Update button measured at bottom 481 in a 480px viewport, and focusing it scrolled the rail to 17 and brought it to 464. * Studio: measure the rail's corner off a card, and drop the stale pointer contract Two fixes to the Playwright suite, both mine to begin with. The corner check measured `fromRight` off the rail's border box. The rail carries the shadow gutter, `-mx-3 px-3`, so its border box sits 4px from the viewport edge while the cards it pads sit at 16. It reported 4 and would have failed at every inspected viewport. Measured at 320x480 and at 921x534, one of the two the pass runs: rail 4, every in-flow card 16. It now takes the right edge from the cards, filtered to the ones in flow, since a dragged loaded models card is `fixed` somewhere else. The bottom still comes off the rail: no vertical margin or padding, and its border box does not move when the cards scroll. Checked at both scroll extremes. The other is a contract this branch already invalidated. The suite asserted the rail takes pointer input exactly when it scrolls, which was #9150 and needed the JS that also placed it. The rail is click-through in every state now, so that check failed whenever the rail scrolled. Verified at 320x480 with the rail actually scrolling, scrollHeight 501 against clientHeight 448: pointer-events none, and a click on the gutter lands on the thread viewport behind it. Both assertions are now unconditional. * CI: drop the install and the artifact dir the deleted rail harness owned The vite install in the banner shard exists for playwright_overlay_rail.py and nothing else, so with the harness gone it is dead. Nothing left in studio-ui-smoke.yml touches npm, npx, node_modules or studio/frontend, so on a frontend-dist cache hit it would run a clean `npm ci` for no consumer. logs/playwright-overlay-rail goes with it: it was only ever written by the two steps this branch removed. Job-level setup-node stays. It predates the harness (#9060) and the installer needs node to build the frontend on a cache miss. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.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
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.

Take the indicator suite off the UI job's critical path
Unsloth UI CI / Chat UI Tests has been timing out at its 30 minute limit on
almost every open PR: 5 of the 6 currently open, at 30m09s to 30m24s. Nothing
was failing. The job was simply longer than the limit, and a limit is a
guillotine, so the steps after the cut never ran at all.
Where it goes: "Cross-browser loaded-models indicator" is 853s of the 30
minutes, three browser engines at about 4.7 minutes each, each booting its own
Unsloth. Everything downstream of it -- the other-engines update banner
regression, both image staged-download regressions, the model-picker
per-model-config suite and the IME / multilingual paste regression -- was
reported "skipped" and has been running on no PR at all.
It is now its own job rather than a step, so it runs beside the rest instead of
inside their budget. That leaves ui-smoke around 16 minutes and the new job
around 15, both with real margin, and the five skipped suites run again. The new
job carries no HF cache and no GGUF: the suite stubs the four /status reads with
page.route, so it needs no model, no GPU and no llama.cpp build.
Moving the work rather than the limit is deliberate. The limit here already went
25 -> 30 for this exact symptom, with the comment on it still describing that
round, so raising it again buys one more release of silence.
Also tests/studio/test_playwright_suites_run_in_ci.py, because these drivers are
standalone scripts that nothing collects: a suite runs only because a workflow
step or a .github/scripts helper names it, and deleting that line leaves every
job green. It fails if any tests/studio/playwright_*.py is named by no workflow
and no CI script. Deleting either the new step or the driver call inside
run-studio-indicator-browser.sh fails it.
It also found two suites already in that state, exempted with a reason each
rather than quietly ignored: playwright_tauri_python_tool_images.py needs the
Tauri desktop shell no runner here builds, and playwright_train_pickers.py needs
a dataset and model resolved through huggingface_hub while the UI workflows boot
API-only. Both are worth wiring up, separately from this.
test_playwright_suites_run_in_ci.py: 3 passed, and 2 failed under each mutation
test_stt_model_search_locator_contract.py + test_composer_rtl_bidi_attribute.py: 34 passed