[staging CI] unslothai/unsloth#7760 - #679
Open
danielhanchen wants to merge 7 commits into
Open
Conversation
The model-config Playwright job has been failing on main since unslothai#7736, on every PR branched after it, for reasons unrelated to what those PRs change. FAIL: could not open run-settings for a model matching 'gemma-3-270m' FAIL: could not reopen run-settings after reload FAIL: Reset button not found in run-settings The behaviour is intentional and the test had not followed it. unslothai#7736 collapses a single-quant row and, as its own comment says, "loads it in one click". The helper clicked the row first and then looked for the gear, so by then the model was loading and the picker had closed. The three failures are one cause: the first is that click, and the other two follow from run-settings never opening. data-model-picker-option sits on the row button and the gear is its sibling, so the gear cannot be reached through the row at all. It is now matched by the model its own aria-label names, which drops the row click and also fixes a second latent bug: `.first` pressed whichever gear came first in the popover rather than the one belonging to the model under test. Bisected on a runner, one commit per run, nothing else changed: 3742317 (parent of unslothai#7736) pass 63307b1 (unslothai#7736) fail origin/main fail origin/main + this change pass
Review on unslothai#7760 is right that a multi-quant parent mounts no inference-settings button until it is expanded, so a gear lookup against it finds nothing. It is not what fails CI today -- that job downloads one quant and Show all quantizations defaults off, so the collapsed path is the only one exercised -- but the helper should not depend on the test model having exactly one quant on disk. Gear first, and only if there is none, click the row and look again. Safe in that order and only in that order: the expandable parent's onClick is toggleGgufExpanded, while the collapsed single-quant row that loads and closes the picker is exactly the row that already had a gear. So the fallback can never reach the row whose click would dismiss the popover. The lookup is also polled now. A row still waiting on its sole-quant probe renders with no gear at all for a moment, and a single miss there would have read as the model having no run-settings.
for more information, see https://pre-commit.ci
…nding Two defects in the fallback, both found reviewing my own change. find_on_device_row matches with Playwright has_text, which is case-insensitive, while find_gear matched with a CSS attribute substring, which is not. So a hint whose case differs from the repo id would find the row and miss the gear -- and then take the new fallback and click a row that loads, which is exactly the bug this PR exists to prevent, now reachable through an env var. Both matchers are case-insensitive now. After expanding, every variant mounts its own gear and all of them carry the repo id, so `.first` over the popover picked an arbitrary quant, possibly one not on disk, and the test would then configure and try to load it. The comment above it claimed the opposite. The second lookup is scoped to the row's own group, with the popover kept only as a fallback. The poll is now a wait_for, which returns the moment the gear mounts instead of sleeping out six fixed intervals -- and the old loop slept after its last attempt too, so a miss cost 3s and the fallback path paid it twice.
Reproduced the main failure locally first: the same three FAILs, from one cause.
With the gear fix the run now passes end to end, which exposed what the cascade
had been hiding.
- A staged edit made in run-settings' first moments is discarded: the panel
re-derives its baseline once mount-time work lands, so Save reports "Default
settings kept" and stores nothing. Measured on gemma-3-270m: fails at 0ms,
passes from 500ms, and neither the input value, the Reset state nor the primary
button label differs across that window, so there is nothing to poll. Bounded
settle wait, with the measurement written down.
- Steps 3 and 3b inherited the popover from step 2, which is why one root cause
reported as three failures and why the re-type-shown regression had been
silently skipping. Each step opens its own now, and the skip is a soft_fail
rather than prose.
- The hidden-model step asserts an absence, so it passed for free when the picker
rendered nothing at all. It now proves the picker is populated first.
- Persistence, reset and migration scanned every stored entry, so another model's
value satisfied them. Scoped to the entry under test; the fallback only fires
when no key has the versioned shape.
- read_configs turned unreadable storage into {}, which several assertions treat
as success, and _count turned a closed page into "selector missing". Both say
what happened now.
- primary_button swept "Load model" first, which substring-matches "Reload
model", so the reload case was found under the wrong name.
- A miss now writes a diagnostics sidecar naming the selector, the option rows and
the gear labels actually present.
Also pinned the hooks the driver depends on in the CPU contract suite. Renaming
one used to type check, lint, pass every unit test and then fail a 25-minute
browser job; the pins catch it in a second.
Unsloth UI CI no longer cancels in-progress runs on main. Pushes land in bursts
there, and four cancelled runs are why this break went unreported for 14 hours.
for more information, see https://pre-commit.ci
Four review items, all mine, all correct. The two contract pins I added were passing vacuously. The trigger hook is carried by triggerDataTour, and my assertion's first term named a prop that does not exist, so it only ever passed on the bare substring -- which the popover's own value contains. It would have gone green with the trigger hook deleted, and the driver's very first click is the thing that would then fail. Same shape for Reset: the fallback searched the whole file, and this file says Reset in two of its own comments, so the pin passed with the button gone. Both now name what they mean, and both fail against a scratch tree with the hook renamed. The expanded-variant lookup passed only the repo hint. Every variant mounts its own gear and all of them carry the repo id, so .first picked an arbitrary quant, possibly one not on disk. The labels are "<repo> <quant>", so it names GGUF_VARIANT now, falling back to the repo-only match so a label format change degrades rather than breaks. entries_for_model matched the repo as a substring of the serialised key, which also matches this repo's other quants -- so a stale entry for one quant could stand in for the one under test and mask its failed save. It parses the key and requires repo and quant to match. Verified it rejects a sibling quant that the substring version accepted.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Disposable CI run for unslothai#7760. Do not merge; closed after CI.