[staging CI] unslothai/unsloth#8884 - #397
danielhanchen wants to merge 15 commits into
Conversation
… wayland WebKit's renderer selection has no session branch: AcceleratedBackingStore::rendererBufferTransportMode reads WEBKIT_DISABLE_DMABUF_RENDERER and WEBKIT_DMABUF_RENDERER_FORCE_SHM and picks the hardware transport on X11 the same way it does on Wayland, so an X11 session on the proprietary driver got no workaround. configure_linux_renderer now probes /proc/driver/nvidia/version before GTK initialization and applies the workaround on either display server, matching WebKit's own isNVIDIA fix for bug 262607. nouveau publishes nothing there and stays on the defaults.
…r overrides Three things, all found while checking this branch against the WebKit sources rather than against descriptions of them. ## The two switches are not interchangeable, and X11 wants the lighter one Traced in webkitgtk-2.50.4, the version the AppImage bundles: DISABLE_DMABUF returns before mode.add(SharedMemory), so the set stays empty, checkRequirements() is false, AcceleratedBackingStore::create() returns nullptr and webkitWebViewBaseEnterAcceleratedCompositingMode() dereferences that behind an ASSERT that release builds compile out. FORCE_SHM returns after the add and leaves a valid backing store. That is not only a smoothness question. block/buzz#3654 reports a SIGSEGV on Ubuntu 24.04, WebKitGTK 2.52.3, X11, NVIDIA module loaded but EGL on Mesa, with DISABLE_DMABUF, and no crash with FORCE_SHM. That is exactly the iGPU-presenting topology the module probe over-triggers on. So the branch now picks per failure mode rather than per GPU. Wayland keeps DISABLE_DMABUF: the failure there is the explicit-sync disconnect, FORCE_SHM routes every commit down the wl_shm path that trips it (WebKit bug 315436), and DISABLE_DMABUF is the switch with reports of fixing Error 71. X11 on 2.44+ takes FORCE_SHM, which drops the hardware transport that fails there without emptying the set. Old WebKitGTK and the missing-GLES AppImage keep the stronger switch. ## Operator overrides win again, without pinning a stale decision Ignoring every FORCE_SHM value on NVIDIA also discarded a deliberate setting, and it was the setting that avoids the crash above. Both variables are operator overrides again. The relaunch case that motivated ignoring them is handled by naming the variable we set in UNSLOTH_WEBKIT_RENDERER_WORKAROUND, so a launch can tell its own inherited output from an instruction, re-decide, and clear the previous variable when the new decision differs. Without that a launch reads its own output back and the first decision is pinned for the life of the process tree; a new test asserts the plan is a fixed point instead. WEBKIT_FORCE_DMABUF_RENDERER is honoured too. It is the opt-out disable-nvidia-dmabuf.patch ships for itself, checked inside isNVIDIA(), which WebKit never reaches once DISABLE_DMABUF is set, so this branch was silently outranking the only lever a Debian or Ubuntu user has. It stands the NVIDIA branch down and nothing else: the missing-GLES fallback answers a packaging failure, not a GPU policy, and that launch cannot render without it. ## Diagnostics An NVIDIA AppImage that cannot load GLES reported only NVIDIA and hid the packaging defect. It now reports both. Two harness docstrings still said this module forces SHM on Wayland; both switches and both display servers are now in scope, so they say so. 19 tests to 27; the whole crate is 415 passing. Verified against the extracted Ubuntu 2.50.4 and Debian 2.52.6 patches (isNVIDIA() before the SharedMemory add, empty set) and 2.53.91 (after it, {SharedMemory}). A differential over 4860 host, session, packaging, inherited-environment and version combinations shows no host losing a workaround it had, and no operator override ignored.
Same decisions, fewer lines: 94 comment lines out, 58 back. Verified behaviour preserving by comparing the plan and the reason string against the previous commit over 420000 input combinations, 0 differences. 415 crate tests pass, rustfmt clean.
…nd-down for PR #8884
Two genuine Codex findings, both reproduced against the shipped sources.
FORCE_SHM alone never reached selection on a patched library: isNVIDIA() returns
before mode.add(SharedMemory) in Ubuntu 2.50.4 and Debian 2.52.6, so the X11 branch
still handed NVIDIA an empty transport set. Its own opt-out is checked first inside
isNVIDIA(), so ForceSharedMemory now sets WEBKIT_FORCE_DMABUF_RENDERER alongside
WEBKIT_DMABUF_RENDERER_FORCE_SHM. Unpatched libraries ignore it, and 2.53.91, which
moved isNVIDIA() after the add, lands on the same {SharedMemory}.
Standing down dropped the ownership marker but left the variable set, so a relaunch
that no longer wanted a workaround kept it, and the launch after that read the now
unmarked value as an operator override and preserved it for good. release_claimed
clears the values we set, never an operator's.
The marker holds the variable list rather than one name, so ownership still covers
a workaround that takes two, and our own FORCE_DMABUF no longer reads back as an
operator opting out.
419 crate tests pass. The 4860-case host matrix still shows no regression, and the
NVIDIA X11 rows now reach {SharedMemory} instead of the empty set.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea5ba1f741
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return StudioAuth( | ||
| access_token=b["access_token"], | ||
| refresh_token=b.get("refresh_token", ""), | ||
| base_url=base_url, | ||
| ) |
There was a problem hiding this comment.
When these examples use the bootstrap password returned by launch_studio() for a fresh non-desktop installation, /api/auth/login returns must_change_password: true, but login() discards that field and proceeds directly to /chat. Studio's route guard then redirects to /change-password, and protected API calls reject the token with 403 Password change required, so the documented fresh-install and side-by-side flows cannot reach the composer. The helper needs to perform the initial password-change flow or require a previously rotated password.
Useful? React with 👍 / 👎.
|
|
||
| payload = { | ||
| "unsloth_auth_token": auth.access_token, | ||
| "unsloth_refresh_token": auth.refresh_token, |
There was a problem hiding this comment.
Studio reads the refresh token from unsloth_auth_refresh_token (studio/frontend/src/features/auth/session.ts), but the kit writes unsloth_refresh_token. Once the access token expires, the SPA cannot refresh the seeded session, so longer test runs are redirected to authentication or start receiving 401 responses despite having obtained a valid refresh token.
Useful? React with 👍 / 👎.
| state = await btn.get_attribute("aria-pressed") | ||
| is_on = (state == "true") | ||
| if is_on != on: | ||
| await btn.click(timeout=timeout_ms) |
There was a problem hiding this comment.
The current composer buttons in studio/frontend/src/components/assistant-ui/thread.tsx expose their state through data-active, not aria-pressed, so this read always returns None and is_on is always false. Enabling a pill happens to work, but set_pill(..., on=False) never clicks it off; for example, tool_pills() leaves Search enabled while executing the Code prompt, changing the request and invalidating the intended isolated tool results.
Useful? React with 👍 / 👎.
| try: | ||
| await stop.wait_for(state="visible", timeout=timeout_ms) | ||
| except Exception: | ||
| pass # Some flows finish faster than the button appears. | ||
| await stop.wait_for(state="hidden", timeout=timeout_ms) |
There was a problem hiding this comment.
If a response completes before the stop button becomes observable, or the request fails without showing it, this call waits the entire timeout_ms before the exception is swallowed. With the default value, every fast or failed turn pauses for 90 seconds even though the subsequent hidden-state check is already satisfied; use a short start timeout or one shared deadline while reserving the long timeout for an observed stream to finish.
Useful? React with 👍 / 👎.
| nohup unsloth studio -H 127.0.0.1 -p 8888 > studio.log 2>&1 & | ||
| for i in $(seq 1 60); do curl -sf http://127.0.0.1:8888/healthz && break; sleep 5; done | ||
| - name: Playwright smoke (studio_test_kit) | ||
| run: PYTHONPATH=.github/scripts python -m studio_test_kit._smoke_ui || true |
There was a problem hiding this comment.
Appending || true discards every nonzero status from the smoke test, including assertion failures for init-script seeding, screenshots, video finalization, and ffmpeg transcoding. Consequently this workflow remains green even when the only Playwright validation it runs is broken, so the step should allow the Python exit status to fail the job.
Useful? React with 👍 / 👎.
… for PR #8884 Membership was not selection. backend_allows_wayland returned true if wayland appeared anywhere in GDK_BACKEND, so x11,wayland read as a Wayland session even though GDK runs on X11 whenever the X display opens. That was only a wrong log line until this branch made the flag pick between the two switches; now it hands an X11 session the empty transport set, which is the configuration with the null backing store report. selected_backend_is_wayland follows gdk_display_manager_open_display in gtk-3-24: g_strsplit on ',' with no trimming, g_str_equal so matching is exact and case sensitive, entries tried in order with the first that opens winning, and '*' expanding to the built-in order, wayland before x11 on Linux. A display cannot be opened before GTK init, so DISPLAY and WAYLAND_DISPLAY stand in for what GDK's own openers read; an explicitly named wayland still counts without WAYLAND_DISPLAY because GDK falls back to the default wayland-0 socket. Four GDK_BACKEND values move against the previous commit and nothing else does: x11,wayland now resolves to X11 when DISPLAY is set, '*' needs a wayland display to pick wayland, and ' wayland' and 'Wayland' match no backend at all. 399 of 15552 combinations, all of them cases where the old rule disagreed with GDK. 421 crate tests pass, three new. The 4860-case host matrix still reports no regression.
…ackend fall through for PR #8884 Two more from the last round, both on code from this branch. The distro opt-out was attached to the workaround, not to the branch that knows why it is needed. The generic Wayland plan uses the same ForceSharedMemory, so a host whose /proc/driver/nvidia is hidden, a container or a confined launch, took that plan and stood the distribution patch down anyway. WebKit would have emptied the transport set there on its own; instead it was pushed onto the wl_shm commit path bug 315436 disconnects on. ForceSharedMemoryOnNvidia now carries the pair and only the confirmed-NVIDIA branch selects it; the generic plan sets FORCE_SHM alone and leaves WebKit's own detection intact. selected_backend_is_wayland also returned on a named wayland without asking whether it could open. GDK skips a backend whose opener fails and tries the next, so wayland,x11 with no compositor runs on X11, and calling it Wayland handed an X11 host the empty transport set. A named wayland now needs a display to win, falling through otherwise, and wl_display_connect(NULL)'s default wayland-0 socket counts as one: wayland_socket_present stats XDG_RUNTIME_DIR before GTK init. A lone wayland with no fallback entry still takes the Wayland workaround, since GTK either finds a compositor or does not start at all. 424 crate tests pass, three new. 144 of 15552 combinations move against 59d7e9b, all of them GDK_BACKEND values naming wayland or the generic Wayland plan; the 4860-case host matrix still reports no regression.
ea5ba1f to
0f0c453
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 775d96bee1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| option = page.get_by_role("option", name=pattern).first | ||
| await option.click(timeout=timeout_ms) |
There was a problem hiding this comment.
When the documented flows seed a Gemini, OpenAI, or Anthropic connection, Studio renders each connected model as a plain <button> rather than an element with role option (studio/frontend/src/features/model-picker/components/model-selector/pickers.tsx:5304-5321). This locator therefore finds nothing and pick_model() times out before every external-provider example can send its first prompt; target the connected-model button or its data-model-picker-option equivalent instead.
Useful? React with 👍 / 👎.
| run: | | ||
| ./install.sh --local --no-torch || pip install -e . | ||
| nohup unsloth studio -H 127.0.0.1 -p 8888 > studio.log 2>&1 & | ||
| for i in $(seq 1 60); do curl -sf http://127.0.0.1:8888/healthz && break; sleep 5; done |
There was a problem hiding this comment.
If the detached Studio process exits or never serves /healthz, all 60 iterations finish with the successful status of the final sleep, so this step remains green. The following _smoke_ui module deliberately starts its own fake server on port 18902 and never accesses Studio on port 8888, meaning even after the separately flagged || true is removed this workflow can pass without a working Studio; explicitly fail after the health loop is exhausted.
Useful? React with 👍 / 👎.
| new_webms = sorted( | ||
| p for p in video_dir.glob("page@*.webm") | ||
| if p not in pre_existing | ||
| ) | ||
| if new_webms: | ||
| final_webm = Path(video_dir) / f"{video_name}.webm" | ||
| if final_webm.exists(): | ||
| final_webm.unlink() | ||
| new_webms[-1].rename(final_webm) |
There was a problem hiding this comment.
When two open_chat calls share video_dir and both take their snapshots before either recording file appears, each later considers both new .webm files its own. The first context to finalize can rename the sibling's recording and delete the other as an "extra," leaving the second context with no video or the wrong video despite the helper explicitly claiming parallel-run safety; obtain the recording path from the context's page/video object rather than inferring ownership from a directory-wide before/after glob.
Useful? React with 👍 / 👎.
| fn wayland_socket_present() -> bool { | ||
| std::env::var_os("XDG_RUNTIME_DIR").is_some_and(|dir| { | ||
| std::path::Path::new(&dir) | ||
| .join(DEFAULT_WAYLAND_SOCKET) | ||
| .exists() |
There was a problem hiding this comment.
With an explicit fallback list such as GDK_BACKEND=wayland,x11, an unset WAYLAND_DISPLAY, and a stale or inaccessible $XDG_RUNTIME_DIR/wayland-0, this existence check reports Wayland even though wl_display_connect(NULL) fails and GDK proceeds to X11. The resulting plan applies the Wayland workaround—on NVIDIA, DisableDmabuf—instead of the X11-specific shared-memory plan, so the probe needs to establish that the socket can actually be opened rather than merely that a directory entry remains.
Useful? React with 👍 / 👎.
775d96b to
112a2e3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ae552faaf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "unsloth_chat_external_providers": json.dumps(provider_entries), | ||
| "unsloth_chat_external_provider_keys": json.dumps(provider_keys), |
There was a problem hiding this comment.
When these examples run against a valid rotated-password Studio with no saved provider configs, the localStorage-only provider is removed before chat becomes ready: CredentialBootstrapGate waits for bootstrapPersistedCredentials() (studio/frontend/src/app/routes/__root.tsx:161-180), and syncExternalProvidersFromBackend() rebuilds the store solely from listProviderConfigs() and returns that list (studio/frontend/src/features/chat/sync-external-providers.ts:187-194,317-318). Consequently the randomly generated provider ID and its key disappear, leaving no external model to select even after the picker locator is corrected; create the provider through the backend API or otherwise preserve it during reconciliation.
Useful? React with 👍 / 👎.
| p = subprocess.run(["xvfb-run", "-a", APPIMAGE], env=env, capture_output=True, | ||
| text=True, timeout=180, check=False) |
There was a problem hiding this comment.
In the Renderer decision in the shipped AppImage step of staging-8884-desktop-linux.yml, the tested GUI is explicitly expected to remain running until killed, but subprocess.run(..., timeout=180) kills it and raises TimeoutExpired; check=False does not suppress timeout exceptions. Since nothing catches that exception, the first fixture aborts after three minutes and none of the renderer assertions execute, so the timeout path must retain the captured output instead of escaping from launch().
Useful? React with 👍 / 👎.
| " const imgs = Array.from(document.querySelectorAll('img'));" | ||
| " const cand = imgs.find(i => i.src && prefixes.some(p => i.src.startsWith(p)));" | ||
| " return cand ? cand.src : null;" |
There was a problem hiding this comment.
When image_generation is composed onto a thread that already contains a generated data-URL image, this find returns that first existing image immediately after the new prompt. The flow then saves the prior turn's bytes and reports success without waiting for the current generation, silently corrupting comparison artifacts; snapshot the matching images before submission or wait for a newly added/last response image.
Useful? React with 👍 / 👎.
| let wayland_display = env(WAYLAND_DISPLAY) | ||
| .map(|display| !display.is_empty()) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
With GDK_BACKEND=wayland,x11, a stale or inaccessible socket named by WAYLAND_DISPLAY, and a working X11 DISPLAY, GDK falls through to X11 but this treats every nonempty Wayland value as an open display. The new connectability probe is fresh evidence for the remaining gap because it checks only $XDG_RUNTIME_DIR/wayland-0, never the socket named here; on NVIDIA this selects the Wayland DisableDmabuf plan instead of the X11 shared-memory plan, potentially recreating the empty-transport crash the branch is intended to avoid.
Useful? React with 👍 / 👎.
1ae552f to
13e70c7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 846d54e189
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| with: | ||
| python-version: '3.12' | ||
| - run: python -m pip install -r studio/backend/requirements/studio.txt pytest pytest-asyncio pytest-timeout python-multipart | ||
| - run: PYTHONPATH=studio/backend python -m pytest studio/backend/tests -q || true |
There was a problem hiding this comment.
The fresh evidence here is that all three newly added backend matrix lanes—Ubuntu, macOS, and Windows—append || true to their only pytest invocation. Any assertion failure, collection error, or test-process crash is therefore discarded, allowing every platform lane to report success without a passing backend suite.
Useful? React with 👍 / 👎.
| let wayland_session = selected_backend_is_wayland( | ||
| env(GDK_BACKEND).as_deref().unwrap_or(OsStr::new("*")), | ||
| wayland_socket, | ||
| env(X11_DISPLAY).is_some_and(|display| !display.is_empty()), |
There was a problem hiding this comment.
When GDK_BACKEND orders X11 before Wayland (for example, x11,wayland), a nonempty but stale or unreachable DISPLAY makes this report X11 even if the live Wayland socket is what GDK subsequently opens. On NVIDIA that selects ForceSharedMemoryOnNvidia instead of the Wayland DisableDmabuf plan, sending the actual Wayland session through the problematic shared-memory path this change is intended to avoid; X11 needs a connectability probe analogous to the Wayland probe rather than a string-presence check.
Useful? React with 👍 / 👎.
| image_path = out_dir / f"{image_basename}.png" | ||
| image_path.write_bytes(await extract_data_url(data_url)) |
There was a problem hiding this comment.
When the accepted response is data:image/jpeg or data:image/webp, this always writes those original bytes to a .png path without converting them. Such providers therefore produce mislabeled comparison artifacts that extension-based viewers and upload tooling may reject or decode incorrectly; derive the suffix from the data URL or actually transcode the bytes to PNG.
Useful? React with 👍 / 👎.
5 participants
Disposable CI run for unslothai/unsloth#8884. Do not merge; closed after CI.