-
-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Studio: allow --secure with --api-only (headless secure API server) and add --api-only to unsloth studio run
#6591
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
40f4925
ab093cf
3c38e30
25f6bc9
a652338
ce11c5d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -893,9 +893,14 @@ def _setup_server_disk_logging(): | |
| def _cloudflare_tunnel_should_start( | ||
| *, cloudflare: bool, host: str, secure: bool, api_only: bool, is_colab: bool | ||
| ) -> bool: | ||
| """Whether to start the Cloudflare tunnel. --secure tunnels a loopback bind too; | ||
| non-secure keeps the 0.0.0.0-only rule. Colab/api-only never tunnel.""" | ||
| return cloudflare and (host == "0.0.0.0" or secure) and not api_only and not is_colab | ||
| """Whether to start the Cloudflare tunnel. --secure exposes only the tunnel | ||
| (loopback bind), so it tunnels even api-only (headless secure API serving); | ||
| otherwise tunnel only a 0.0.0.0 bind, never api-only (Tauri) or Colab.""" | ||
| if is_colab or not cloudflare: | ||
| return False | ||
| if secure: | ||
| return True | ||
|
Comment on lines
+901
to
+902
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This review ran against 3c38e30, which is the commit just before the fix. It is already addressed in 25f6bc9 (now 25f6bc9..a652338 after a pre-commit.ci reformat): secure api-only uses the non-Tauri policy. cors_origins_for_mode(api_only, secure) returns the Tauri allowlist only when |
||
| return host == "0.0.0.0" and not api_only | ||
|
|
||
|
|
||
| def _apply_cli_tool_policy(enable_tools: "Optional[bool]") -> None: | ||
|
|
@@ -919,6 +924,7 @@ def run_server( | |
| cloudflare: bool = True, | ||
| secure: bool = False, | ||
| enable_tools: "Optional[bool]" = None, | ||
| emit_tauri_port: bool = True, | ||
| ): | ||
| """ | ||
| Start the FastAPI server. | ||
|
|
@@ -932,6 +938,9 @@ def run_server( | |
| llama_parallel_slots: parallel slots for llama-server | ||
| enable_tools: explicit --enable-tools/--disable-tools policy; None leaves | ||
| the default (tools on, per-request enable_tools honored) | ||
| emit_tauri_port: print the machine-readable TAURI_PORT line the desktop | ||
| app parses from stdout; the headless `run --api-only` path turns it | ||
| off so it does not pollute the documented URL/API-key banner | ||
|
|
||
| Note: | ||
| Signal handlers are NOT registered here so embedders (e.g. Colab) keep | ||
|
|
@@ -974,9 +983,13 @@ def run_server( | |
| if _session_log is not None and not silent: | ||
| print(f"Session log: {_session_log}") | ||
|
|
||
| # Set env var BEFORE importing main so CORS middleware picks it up. | ||
| # Set env vars BEFORE importing main so CORS middleware picks them up. | ||
| # secure api-only is a remote server behind Cloudflare, so it keeps the | ||
| # any-origin CORS profile; plain api-only stays locked to the Tauri app. | ||
| if api_only: | ||
| os.environ["UNSLOTH_API_ONLY"] = "1" | ||
| if secure: | ||
| os.environ["UNSLOTH_SECURE"] = "1" | ||
|
|
||
| import nest_asyncio | ||
|
|
||
|
|
@@ -1158,7 +1171,8 @@ def _run(): | |
| atexit.register(terminate_all) | ||
|
|
||
| # Output port for Tauri (api-only), only after sockets bind and startup done. | ||
| if api_only: | ||
| # The headless `run --api-only` path opts out so it does not leak this line. | ||
| if api_only and emit_tauri_port: | ||
| print(f"TAURI_PORT={port}", flush = True) | ||
|
|
||
| # Free trycloudflare.com tunnel for 0.0.0.0 binds (the raw ip:port is often | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -776,6 +776,16 @@ def studio_default( | |
| err = True, | ||
| ) | ||
| raise typer.Exit(2) | ||
| # Same for --api-only: dropping it here would silently serve the UI. | ||
| if api_only: | ||
| typer.echo( | ||
| f"Error: --api-only on `unsloth studio` applies to the " | ||
| f"plain-server path only. For `unsloth studio " | ||
| f"{ctx.invoked_subcommand}`, put it after the subcommand: " | ||
| f"`unsloth studio {ctx.invoked_subcommand} --api-only ...`", | ||
| err = True, | ||
| ) | ||
| raise typer.Exit(2) | ||
| return | ||
|
|
||
| # --secure requires the tunnel; force a loopback bind. | ||
|
|
@@ -1031,6 +1041,12 @@ def run( | |
| host: str = typer.Option("127.0.0.1", "--host", "-H"), | ||
| # `-f` removed (clustered `-fa`/`-fit*`); studio_default keeps it. | ||
| frontend: Optional[Path] = typer.Option(None, "--frontend"), | ||
| api_only: bool = typer.Option( | ||
| False, | ||
| "--api-only", | ||
| help = "Serve only the API (no web UI), for a headless model server. " | ||
| "Pairs with --secure to expose the API over the Cloudflare link alone.", | ||
|
Comment on lines
+1044
to
+1048
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the user places the new flag before the subcommand, e.g. Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 25f6bc9. The parent studio_default callback now rejects --api-only when a subcommand is invoked, matching the existing guidance for --parallel/--secure/--verbose: it exits 2 and tells the user to put the flag after the subcommand ( |
||
| ), | ||
| silent: bool = typer.Option(False, "--silent", "-q"), | ||
| enable_tools: Optional[bool] = typer.Option( | ||
| None, | ||
|
|
@@ -1214,6 +1230,8 @@ def run( | |
| args.append("--load-in-4bit" if load_in_4bit else "--no-load-in-4bit") | ||
| if frontend: | ||
| args.extend(["--frontend", str(frontend)]) | ||
| if api_only: | ||
| args.append("--api-only") | ||
| if silent: | ||
| args.append("--silent") | ||
| # Forward the resolved tool policy so the child doesn't re-resolve. | ||
|
|
@@ -1262,9 +1280,13 @@ def run( | |
| host = host, | ||
| port = port, | ||
| silent = True, | ||
| api_only = api_only, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in ce11c5d. run_server now takes emit_tauri_port (default True, so the Tauri/desktop and studio_default paths are unchanged), and the TAURI_PORT= line is gated on |
||
| llama_parallel_slots = parallel, | ||
| cloudflare = cloudflare, | ||
| secure = secure, | ||
| # Headless serving prints its own URL/API-key banner; the Tauri-only | ||
| # TAURI_PORT line would corrupt that machine-parseable output. | ||
| emit_tauri_port = False, | ||
| ) | ||
| if frontend is not None: | ||
| run_kwargs["frontend_path"] = frontend | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -361,6 +361,29 @@ def test_studio_default_rejects_parallel_when_subcommand_invoked(): | |
| ), f"error message must show the corrected invocation; got: {combined}" | ||
|
|
||
|
|
||
| def test_studio_default_rejects_api_only_when_subcommand_invoked(): | ||
| """`unsloth studio --api-only run ...` would silently serve the UI (the | ||
| parent's --api-only never reaches run). The callback rejects with exit 2 | ||
| and points at the subcommand flag.""" | ||
| studio_mod = _load_run_command() | ||
| import typer as _typer | ||
|
|
||
| app = _typer.Typer() | ||
| app.add_typer(studio_mod.studio_app, name = "studio") | ||
|
|
||
| runner = CliRunner() | ||
| result = runner.invoke(app, ["studio", "--api-only", "run", "--model", "X"]) | ||
| assert result.exit_code == 2, ( | ||
| f"expected exit 2 when --api-only is on studio group with a " | ||
| f"subcommand invoked; got {result.exit_code}; output={result.output!r}" | ||
| ) | ||
| combined = (result.output or "") + (getattr(result, "stderr", "") or "") | ||
| assert "--api-only" in combined, combined | ||
| assert ( | ||
| "run --api-only" in combined | ||
| ), f"error message must show the corrected invocation; got: {combined}" | ||
|
|
||
|
|
||
| def test_studio_default_default_parallel_with_subcommand_does_not_error(): | ||
| """Omitting --parallel on the group must still let subcommands | ||
| run; the group's default 1 is benign.""" | ||
|
|
@@ -445,3 +468,81 @@ def fake_run_server(**kwargs): | |
| assert ( | ||
| captured.get("llama_parallel_slots") == value | ||
| ), f"run_server got llama_parallel_slots={captured.get('llama_parallel_slots')!r}, expected {value}" | ||
|
|
||
|
|
||
| # --api-only: serve API only (no UI). Both re-exec and in-venv paths must carry it. | ||
|
|
||
|
|
||
| def test_api_only_option_is_registered(): | ||
| studio_mod = _load_run_command() | ||
| import inspect | ||
|
|
||
| opt = inspect.signature(studio_mod.run).parameters["api_only"].default | ||
| assert "--api-only" in set(getattr(opt, "param_decls", []) or []) | ||
| assert getattr(opt, "default", None) is False # opt-in; plain run keeps the UI | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "extra,present", | ||
| [ | ||
| (["--api-only"], True), | ||
| (["--secure", "--api-only"], True), # secure headless path | ||
| ([], False), | ||
| ], | ||
| ) | ||
| def test_reexec_forwards_api_only(monkeypatch, extra, present): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When pytest imports this module, this second Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in ce11c5d. That duplicate definition was shadowing the parametrized test_reexec_forwards_api_only below it, so the secure case was never collected. I removed the duplicate (and the redundant omit-by-default test, already covered by the parametrized ([], False) case) and folded the secure headless path in as a param: (['--secure', '--api-only'], True). One canonical test now, and the secure case is collected. |
||
| """`--api-only` (and only when typed) must reach the re-exec'd child.""" | ||
| result, captured = _invoke_run(monkeypatch, _BASE + extra) | ||
| assert len(captured) == 1, result.output | ||
| argv = captured[0]["argv"] | ||
| assert ("--api-only" in argv) is present, argv | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("extra,expected", [(["--api-only"], True), ([], False)]) | ||
| def test_in_venv_path_passes_api_only_to_run_server(monkeypatch, extra, expected): | ||
| """In-venv path must forward --api-only to run_server(api_only=...).""" | ||
| studio_mod = _load_run_command() | ||
|
|
||
| fake_venv = Path("/fake/studio/venv/unsloth_studio") | ||
| monkeypatch.setattr(sys, "prefix", str(fake_venv)) | ||
| monkeypatch.setattr(studio_mod, "STUDIO_HOME", fake_venv.parent) | ||
|
|
||
| from unsloth_cli import _tool_policy as _tp_mod | ||
|
|
||
| monkeypatch.setattr( | ||
| _tp_mod, | ||
| "resolve_tool_policy", | ||
| lambda host, flag, yes, silent: False if flag is None else bool(flag), | ||
| ) | ||
|
|
||
| captured: dict = {} | ||
|
|
||
| def fake_run_server(**kwargs): | ||
| captured.update(kwargs) | ||
| raise _RunServerCaptured(kwargs) | ||
|
|
||
| fake_backend_run = sys.modules.setdefault( | ||
| "studio.backend.run", _types_module("studio.backend.run") | ||
| ) | ||
| fake_backend_run.run_server = fake_run_server | ||
| fake_backend_run._resolve_external_ip = lambda: "127.0.0.1" | ||
| monkeypatch.setattr(studio_mod, "_RUN_MODULE", fake_backend_run) | ||
|
|
||
| import typer as _typer | ||
|
|
||
| app = _typer.Typer() | ||
| app.command( | ||
| context_settings = { | ||
| "allow_extra_args": True, | ||
| "ignore_unknown_options": True, | ||
| }, | ||
| )(studio_mod.run) | ||
| CliRunner().invoke(app, _BASE + extra, catch_exceptions = True) | ||
|
|
||
| assert ( | ||
| captured.get("api_only") is expected | ||
| ), f"run_server got api_only={captured.get('api_only')!r}, expected {expected}" | ||
| # Headless serving must suppress the Tauri-only TAURI_PORT line. | ||
| assert ( | ||
| captured.get("emit_tauri_port") is False | ||
| ), f"run_server got emit_tauri_port={captured.get('emit_tauri_port')!r}, expected False" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With
--secure --api-only, this branch now publishes an API-only server through Cloudflare, butrun_server(api_only=True)setsUNSLOTH_API_ONLY=1before importingmain, andstudio/backend/main.py:858-867switches CORS from*to a Tauri/localhost-only allowlist. Browser-based remote API clients calling the new Cloudflare URL from any other origin will fail preflight even with a valid API key, whereas--securewithout--api-onlyworks. If this mode is meant to support remote API callers, keep the UI disabled without enabling the Tauri-only CORS profile or allow the tunnel origin.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 25f6bc9. Secure api-only is a remote server behind Cloudflare, so it now keeps the any-origin CORS profile; the Tauri-only lockdown still applies to plain local api-only. The decision is factored into cors_origins_for_mode(api_only, secure) (returns the Tauri allowlist only when api_only and not secure, else [""]), and run_server exports UNSLOTH_SECURE before importing main so main.py can tell the two apart. With allow_credentials=True and [""], Starlette reflects the request Origin, so remote browser preflight succeeds. Added test_cors_origins_for_mode and test_run_server_exports_secure_env_for_cors.