Studio: auto-recover when shadowed 'unsloth' on PATH hides the frontend dist#5782
Conversation
…nd dist
The CLI launcher derives `_PACKAGE_ROOT` from where `unsloth_cli` imports
from, and `studio/backend/run.py` derives its default `frontend_path` from
`Path(__file__).resolve().parent.parent / "frontend" / "dist"`. When
another `unsloth` (a separate venv with `pip install unsloth`, a system
install, an older venv earlier on PATH) wins `which unsloth`, both
resolve into a site-packages tree that ships frontend source files but no
vite-built `dist/`. The backend warned `[WARNING] Frontend not found at
...` and then happily served 200 on every `/api/*` route while returning
`{"detail":"Not Found"}` on `/`. The 404 was silent to users -- the
process was healthy, the log line scrolled by, and the only symptom was a
blank browser tab.
This is a real situation: many devboxes carry a workspace venv with
`unsloth` installed years before the user runs `curl|sh` to install
Studio. The installer-managed binary at `~/.local/bin/unsloth` exists
but loses to the older venv on PATH order.
Three layers of fix, additive:
Layer C -- runtime auto-discovery (unsloth_cli + run.py)
The CLI now resolves `--frontend` explicitly before spawning `run.py`,
probing in order: package-local default, installer venv site-packages
(`$STUDIO_HOME/unsloth_studio/lib/python*/site-packages/...` and the
Windows `Lib/site-packages/...` equivalent), and editable-install source
roots read from `__editable___*_finder.py` MAPPING dicts in the installer
venv. `run.py` does the same probe as a backstop for direct `python
run.py` invocations.
Layer E -- loud structured error
The silent `[WARNING]` is replaced with a `SystemExit` that names every
candidate path tried and lists the four one-line fixes (run the absolute
path, pass `--frontend`, pass `--api-only`, reinstall). Suppressed only
in `--api-only` mode where no UI is served by design.
Layer F -- installer self-check (install.sh + install.ps1)
At the tail of install, both installers compare `command -v unsloth`
(POSIX) / `Get-Command unsloth` (PowerShell) against the just-installed
binary. If a different path wins, a yellow `warning` block names the
shadowing binary and prints the alias / absolute-path / PATH-reorder
fixes. install.sh uses the venv Python for path canonicalization so it
also works on macOS (BSD `readlink` has no `-f`).
Cross-platform notes:
- Glob patterns probe both `lib/python*/site-packages` (POSIX) and
`Lib/site-packages` (Windows).
- Canonical-binary path branches on `sys.platform == "win32"` to pick
`unsloth.exe` over `unsloth`.
- install.sh fixed for macOS; install.ps1 is the Windows analog.
Tests: `studio/backend/tests/test_frontend_resolution.py` covers five
cases via AST-load of the helpers (no uvicorn / FastAPI import needed,
matching `test_host_defaults.py`'s style):
1. Resolver returns None when nothing exists anywhere.
2. Resolver picks the first existing candidate when the default works.
3. Fallback to `$UNSLOTH_STUDIO_HOME` site-packages dist when the default
is missing.
4. Fallback to an editable-install source root via MAPPING parsing.
5. Resolver tolerates a non-existent `$UNSLOTH_STUDIO_HOME`.
All 5 new + 2 existing host-default tests pass.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Code Review
This pull request introduces fallback resolution logic for locating the built frontend dist directory in both the backend (run.py) and CLI (studio.py). This addresses issues where another unsloth binary on the PATH shadows the installer's binary, leading to missing frontend assets or silent 404 errors. It also adds warnings during installation (install.sh and install.ps1) when a PATH conflict is detected. The review feedback points out a potential crash in both run.py and studio.py if ast.literal_eval parses a non-dictionary literal, and suggests adding explicit type checks (isinstance(mapping, dict)) before calling .get().
| try: | ||
| mapping = ast.literal_eval(m.group(1)) | ||
| except (SyntaxError, ValueError): | ||
| continue | ||
| studio_pkg = mapping.get("studio") | ||
| if studio_pkg: | ||
| out.append(Path(studio_pkg) / "frontend" / "dist") |
There was a problem hiding this comment.
If ast.literal_eval parses a literal that is not a dictionary (such as a set, list, or None), calling .get() on it will raise an AttributeError and crash the backend startup. We should explicitly verify that mapping is a dictionary before attempting to access its keys.
| try: | |
| mapping = ast.literal_eval(m.group(1)) | |
| except (SyntaxError, ValueError): | |
| continue | |
| studio_pkg = mapping.get("studio") | |
| if studio_pkg: | |
| out.append(Path(studio_pkg) / "frontend" / "dist") | |
| try: | |
| mapping = ast.literal_eval(m.group(1)) | |
| except (SyntaxError, ValueError): | |
| continue | |
| if isinstance(mapping, dict): | |
| studio_pkg = mapping.get("studio") | |
| if studio_pkg: | |
| out.append(Path(studio_pkg) / "frontend" / "dist") |
There was a problem hiding this comment.
Fixed in 499e7d8. Both call sites (studio/backend/run.py:558 and unsloth_cli/commands/studio.py:234) now guard with isinstance(mapping, dict) and continue on a non-dict literal, so the resolver keeps probing the remaining finder files. Setuptools's template only emits dict literals today, so this is defensive against a future change rather than a live bug, but the guard is one line and prevents the crash mode. Added a regression test (test_resolver_does_not_crash_on_non_dict_mapping_literal) that writes a bad finder (MAPPING is a set literal) alongside a good one and asserts the resolver returns the good finder's dist path. Without the guard the test crashes with AttributeError; with it, 11/11 tests pass.
| try: | ||
| mapping = ast.literal_eval(m.group(1)) | ||
| except (SyntaxError, ValueError): | ||
| continue | ||
| studio_pkg = mapping.get("studio") | ||
| if studio_pkg: | ||
| yield Path(studio_pkg).parent |
There was a problem hiding this comment.
If ast.literal_eval parses a literal that is not a dictionary (such as a set, list, or None), calling .get() on it will raise an AttributeError and crash the CLI. We should explicitly verify that mapping is a dictionary before attempting to access its keys.
| try: | |
| mapping = ast.literal_eval(m.group(1)) | |
| except (SyntaxError, ValueError): | |
| continue | |
| studio_pkg = mapping.get("studio") | |
| if studio_pkg: | |
| yield Path(studio_pkg).parent | |
| try: | |
| mapping = ast.literal_eval(m.group(1)) | |
| except (SyntaxError, ValueError): | |
| continue | |
| if isinstance(mapping, dict): | |
| studio_pkg = mapping.get("studio") | |
| if studio_pkg: | |
| yield Path(studio_pkg).parent |
There was a problem hiding this comment.
Fixed in 499e7d8 (see reply on the run.py thread for the full rationale; same one-line guard, same regression test covers both call sites).
…h hint, broader tests) Four parallel platform reviews (Windows, Linux, macOS, general) on the initial commit surfaced a small batch of correctness items, all addressed here: Windows install.ps1 (medium severity, false positive on every install): The user-facing shim at $StudioHome\bin\unsloth.exe is a hardlink to $VenvDir\Scripts\unsloth.exe (created at line 1582). Resolve-Path does not de-duplicate hardlinks, so the previous string compare always saw the two paths as different and the new "another 'unsloth' wins on PATH" warning would fire on every fresh Windows install. Switched to content-hash equality via Get-FileHash, which collapses hardlinks, symlinks, and identical copies to a single identity. Also restricted the probe to Get-Command -CommandType Application so PowerShell aliases / functions / scripts named "unsloth" don't false-trigger. Windows run.py SystemExit hint (medium severity, defeats the recovery UX): The structured error printed Path(STUDIO_HOME)/"unsloth_studio"/"bin"/ "unsloth.exe" on every platform, but on Windows the installer places the shim at $STUDIO_HOME/bin/unsloth.exe (no unsloth_studio segment) and the venv binary at $STUDIO_HOME/unsloth_studio/Scripts/unsloth.exe. The hint pointed at a non-existent path on Windows. Branch on sys.platform == "win32" to emit the real shim location; Linux / macOS keep the unsloth_ studio/bin/unsloth layout. MAPPING regex robustness (low): [^\n]* silently failed if a future setuptools / black reformat wrapped the MAPPING dict across multiple lines. Tightened to [^}]* + re.DOTALL, which still rejects nested dicts (setuptools never emits those for editable installs) but tolerates either single- or multi-line literals. install.sh broken-venv edge case (low, macOS reviewer): Previously _canon fell back to echoing the raw input when the venv python failed, which would make two symlinked-but-identical paths look different and false-trigger the warning. Now _canon returns empty on failure and the caller skips the whole comparison if either side is unresolvable. argparse default + log readability (nits): run.py's argparse --frontend default now reuses the module-level _DEFAULT_FRONTEND_PATH constant so it stays in lockstep with run_server's default. The [OK] log message resolves the chosen path so support output is always absolute. Tests grow from 5 to 8 in studio/backend/tests/test_frontend_resolution. py (10/10 with the existing host-default tests): - Windows-layout fallback: Lib/site-packages with capital L. - Multi-line MAPPING dict: locks in the [^}]* + re.DOTALL behaviour. - SystemExit message contract: every actionable fix string and the attempted-paths list must appear; pins the user-facing recovery message so a future refactor doesn't drop a bullet. End-to-end re-verified on this box: shadowing workspace_22/bin/unsloth still serves 200 on / through the editable-finder fallback, with the follow-up resolve-then-log change yielding [OK] Frontend loaded from /mnt/disks/unslothai/ubuntu/unsloth/studio/frontend/dist. Out of scope (called out by reviewers but deferred): - _resolve_frontend_path candidate ordering still tries _PACKAGE_ROOT first. For the rare case where a shadowing install carries an older built dist, this serves the stale UI instead of the fresh one. Fix is non-trivial (the --local workflow intentionally wants _PACKAGE_ROOT to win when the cloned repo is the source of truth), so leaving it for a follow-up. - studio/backend/colab.py still bails out on missing frontend instead of routing through the new resolver. Pre-existing behaviour, separate PR. - _resolve_frontend_path is duplicated across run.py and unsloth_cli/ commands/studio.py. Minor maintenance concern; consolidation is natural in a later refactor.
|
Follow-up commit What changed
TestsThree new tests, 8 total in
Out of scope (called out, deferred)
End-to-end re-verified locally: shadowing |
Addresses gemini-code-assist[bot] high-priority inline review on PR 5782
flagging that `mapping.get('studio')` could raise AttributeError if the
MAPPING regex matched a brace-delimited literal that ast.literal_eval
parsed as a non-dict (set, list, None). The regex `\{[^}]*\}` happily
matches `{1, 2, 3}` and literal_eval returns a set; the previous code
then crashed on .get().
Setuptools's editable-install template only emits dict literals so this
is defensive rather than a live bug, but the guard is one line per call
site and prevents a future template change from taking out backend
startup or CLI invocation.
Both call sites (studio/backend/run.py:558 and
unsloth_cli/commands/studio.py:234) now bail out on the finder file when
isinstance(mapping, dict) is False; the resolver keeps probing the
remaining finders, so a malformed entry in one finder cannot poison the
discovery of a good one elsewhere.
Adds test_resolver_does_not_crash_on_non_dict_mapping_literal to
test_frontend_resolution.py, which writes one bad finder (MAPPING is a
set literal) alongside one good finder (MAPPING is a real dict) and
asserts the resolver returns the good finder's dist path. Without the
guard this test crashes with AttributeError; with the guard it passes.
11/11 tests green.
Resolve studio.py conflict by keeping both: - PR unslothai#5737's `_PARALLEL_*` constants block - main's `_iter_editable_studio_source_roots` + `_find_frontend_dist` helpers (PR unslothai#5782). run.py auto-merged cleanly. 215 PR tests still pass.
Summary
Fixes a silent 404 on
/when anotherunslothbinary on PATH shadows the installer's shim. The CLI derives_PACKAGE_ROOTfrom whereunsloth_cliwas imported, andstudio/backend/run.pyderives its defaultfrontend_pathas a sibling of__file__. When a non-installerunslothwins on PATH (a workspace venv withpip install unsloth, a system install, etc.), both resolve into a site-packages tree that ships frontend source but no vite-builtdist/. The backend logged[WARNING] Frontend not found at ...and then served 200 on every/api/*route while returning{"detail":"Not Found"}on/. The 404 was invisible to users since/api/healthlooked fine.This patch adds runtime auto-discovery, a loud error when discovery fails, and an installer self-check that warns about PATH shadowing.
Repro before the fix
What changes
Layer C: runtime auto-discovery
unsloth_cli/commands/studio.pygains_find_frontend_dist()and_iter_editable_studio_source_roots(). Before spawningrun.py, the CLI resolves--frontendexplicitly by probing:_PACKAGE_ROOT/studio/frontend/dist$UNSLOTH_STUDIO_HOME/unsloth_studio/lib/python*/site-packages/studio/frontend/dist(POSIX)$UNSLOTH_STUDIO_HOME/unsloth_studio/Lib/site-packages/studio/frontend/dist(Windows)__editable___*_finder.pyMAPPING dicts in the installer venvstudio/backend/run.pygains the same probe as a backstop for directpython run.pyinvocations.Layer E: loud structured error
The silent
[WARNING]becomes aSystemExitthat lists every candidate tried and the four one-line fixes (absolute path,--frontend,--api-only, reinstall). Skipped in--api-onlymode where no UI is served.Layer F: installer self-check
install.shandinstall.ps1comparecommand -v unsloth/Get-Command unslothagainst the just-installed binary at the end of install. If a different path wins, both print a yellow warning block naming the shadowing binary plus the alias / absolute-path / PATH-reorder fixes. install.sh uses the freshly-created venv Python for path canonicalization (BSD readlink on macOS has no-f, so the previous attempt withreadlink -fwould have degraded on Mac).Cross-platform compat
lib/python*/site-packages(POSIX) andLib/site-packages(Windows).sys.platform == "win32"to pickunsloth.exeoverunsloth.step/substephelpers.Tests
studio/backend/tests/test_frontend_resolution.pyadds 5 tests via AST-loading the helpers (same style astest_host_defaults.py, no uvicorn / FastAPI imports needed):$UNSLOTH_STUDIO_HOMEsite-packages dist when the default is missing.$UNSLOTH_STUDIO_HOME.All 5 new + 2 existing tests pass:
Test plan
pytest studio/backend/tests/test_frontend_resolution.py studio/backend/tests/test_host_defaults.pypasses (7 tests).pre-commit run --files install.sh install.ps1 studio/backend/run.py unsloth_cli/commands/studio.py studio/backend/tests/test_frontend_resolution.pypasses.curl -s http://127.0.0.1:PORT/now returns 200 with the real index.html via the shadowed binary -- no PATH or alias change required.[OK] Frontend loaded from /path/to/editable/clone/studio/frontend/distappears in the log, confirming the editable-install fallback path fires correctly.Files touched