From 3a549d336624c117f6f8c61659dc8707d505c04d Mon Sep 17 00:00:00 2001 From: Alpamys Date: Wed, 8 Jul 2026 19:26:57 +0500 Subject: [PATCH] fix: resolve full-tree code-review findings (6 HIGH + MEDIUM/LOW) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address every finding from the v0.71.32 review round. No version bump — lands under CHANGELOG [Unreleased] for the next patch. HIGH: - serve: vLLM no longer forces trust_remote_code=True; resolved once for all backends via the v0.36.0 default-deny gate + warning panel. - serve: multi-adapter serving actually loads + switches adapters (_load_named_adapters + lock-guarded _adapter_scope on both the non-stream and stream generation paths); was a silent no-op. - data/loader: vision image paths are containment-checked against image_dir (mirrors the audio loader) — {"image": "/etc/passwd"} dropped. - train: --dry-run skips the accelerate re-exec; also forwards --minillm-on-policy / --capture-activations / --capture-prompts. - trainer/mlx_sft: build a real AdamW instead of optimizer=None. - commands/data: escape dataset/HF-derived strings in inspect/preview/search. ASR (v0.71.32 review): metric calls moved inside the per-row try (no batch-crash on an oversized reference), control-strip + escape on skip warnings, transcript-label cap to Whisper's 448 + >30s audio warning, upfront --asr-task validation, exit 2 on all-skipped, fp16 fallback on pre-Ampere GPUs. MEDIUM/LOW: distill KD term causal-shift to match the CE term; constant- time UI Bearer-token compare + locked _train_process read; load_config_from_string raises ValueError (not TypeError) on non-mapping YAML; soup runs escapes config-derived fields; vscode_setup + lr_finder use the centralised atomic, symlink-rejecting writer. Tests: +28 in tests/test_review_fixes_v07133.py; updated 4 existing tests that encoded pre-fix behavior (2 vision, 1 distill, 1 vLLM). Full suite 15569 passed, 0 failed, 123 skipped; ruff clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 36 ++ src/soup_cli/commands/data.py | 38 +- src/soup_cli/commands/infer.py | 67 ++- src/soup_cli/commands/runs.py | 29 +- src/soup_cli/commands/serve.py | 192 +++++++-- src/soup_cli/commands/train.py | 53 ++- src/soup_cli/config/loader.py | 7 + src/soup_cli/data/loader.py | 24 +- src/soup_cli/trainer/asr.py | 46 +- src/soup_cli/trainer/distill.py | 15 + src/soup_cli/trainer/mlx_sft.py | 9 +- src/soup_cli/ui/app.py | 10 +- src/soup_cli/utils/lr_finder.py | 14 +- src/soup_cli/utils/vllm.py | 9 +- src/soup_cli/utils/vscode_setup.py | 34 +- tests/test_code_review_high.py | 33 +- tests/test_review_fixes_v07133.py | 662 +++++++++++++++++++++++++++++ tests/test_vision.py | 36 +- tests/test_vllm_serve.py | 1 + 19 files changed, 1176 insertions(+), 139 deletions(-) create mode 100644 tests/test_review_fixes_v07133.py diff --git a/CHANGELOG.md b/CHANGELOG.md index cfe7daf..72ae137 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,42 @@ reproducing 70+ versions of notes. ## [Unreleased] +### Fixed +- **`soup serve --backend vllm` no longer force-enables `trust_remote_code`.** The + vLLM path now goes through the same `--trust-remote-code` default-deny gate (and + warning panel) as the transformers backend, so serving an untrusted repo never + executes its code silently. +- **Multi-adapter serving (`soup serve --adapters name=path`) now actually switches + adapters.** The named adapters are loaded into the model and selected per request + (via `POST /v1/adapters/activate/{name}` or the request `adapter` field); + previously every request silently ran the startup model. The base model is served + when no adapter is selected. +- **Vision datasets reject out-of-directory image paths.** `llava` / `sharegpt4v` + rows are containment-checked against `image_dir` (mirroring the audio loader), so a + crafted `{"image": "/etc/passwd"}` row can no longer read arbitrary local files. +- **`soup train --dry-run --gpus N` no longer launches a real multi-GPU run.** The + accelerate re-exec is skipped under `--dry-run`. The re-exec also now forwards + `--minillm-on-policy`, `--capture-activations`, and `--capture-prompts` (previously + dropped on multi-GPU runs). +- **MLX SFT now builds a real optimizer** (`AdamW` from the configured LR) instead of + passing `optimizer=None`, which left the model untrained. +- **`soup data inspect` / `preview` / `search` escape dataset- and Hub-derived text** + so a stray `[/]` no longer crashes the command and a crafted `[link=…]` tag can't + render a phishing hyperlink. `soup runs` list/show escape config-derived fields too. +- **`soup infer --task asr` hardening** — an oversized reference no longer crashes the + whole batch after transcription (that row's metric is skipped); an all-skipped run + exits non-zero instead of reporting success; `--asr-task` is validated upfront; and + dataset-derived filenames are control-stripped before printing. ASR training now + caps transcript labels to Whisper's decoder limit, warns on >30 s audio, and picks + fp16 on pre-Ampere GPUs instead of hardcoding bf16. +- **Knowledge-distillation KD term aligns with the CE term.** The token-level + divergence is now computed over causal-shifted positions, so the distillation signal + covers exactly the trained tokens (previously off by one). +- **Miscellaneous robustness** — `load_config_from_string` raises `ValueError` (not + `TypeError`) on a non-mapping YAML document; the Web UI Bearer-token check is + constant-time; and `soup doctor --vscode` / the LR-finder report use the centralised + atomic, symlink-rejecting writer. + ## [0.71.32] - 2026-07-07 ### Added diff --git a/src/soup_cli/commands/data.py b/src/soup_cli/commands/data.py index c161d52..3efc65d 100644 --- a/src/soup_cli/commands/data.py +++ b/src/soup_cli/commands/data.py @@ -53,12 +53,20 @@ def inspect( # Print sample rows if rows > 0 and len(data) > 0: + # Escape dataset-derived cell content + column names: a stray '[/]' in + # ordinary data crashes Rich with MarkupError; a crafted '[link=...]' + # renders a phishing hyperlink. Mirrors `soup data review`. + from rich.markup import escape as _escape + console.print(f"\n[bold]Sample rows ({min(rows, len(data))}):[/]") sample_table = Table(show_lines=True) for col in result["columns"][:5]: # max 5 columns - sample_table.add_column(col, max_width=60) + sample_table.add_column(_escape(str(col)), max_width=60) for row in data[: min(rows, len(data))]: - values = [str(row.get(col, ""))[:60] for col in result["columns"][:5]] + values = [ + _escape(str(row.get(col, ""))[:60]) + for col in result["columns"][:5] + ] sample_table.add_row(*values) console.print(sample_table) @@ -1102,16 +1110,20 @@ def search_datasets( table.add_column("Likes", justify="right") table.add_column("Tags", max_width=30) + # HF-hub metadata (ids, tags) is attacker-authored — escape before it + # reaches a Rich table (MarkupError crash / phishing-link injection). + from rich.markup import escape as _escape + for ds_item in datasets[:limit]: ds_tags = getattr(ds_item, "tags", []) or [] - tag_str = ", ".join(ds_tags[:5]) + tag_str = ", ".join(str(t) for t in ds_tags[:5]) if len(ds_tags) > 5: tag_str += "..." table.add_row( - ds_item.id, + _escape(str(ds_item.id)), _format_count(getattr(ds_item, "downloads", 0) or 0), _format_count(getattr(ds_item, "likes", 0) or 0), - tag_str, + _escape(tag_str), ) console.print(table) @@ -1137,22 +1149,26 @@ def preview_dataset( ) raise typer.Exit(1) - table = Table(title=f"Dataset: {info['id']}") + # HF-hub metadata (id, description, tags, feature/split names) is + # attacker-authored — escape before it reaches a Rich table. + from rich.markup import escape as _escape + + table = Table(title=f"Dataset: {_escape(str(info['id']))}") table.add_column("Field", style="bold") table.add_column("Value", max_width=80) - table.add_row("ID", info["id"]) + table.add_row("ID", _escape(str(info["id"]))) desc = info["description"] if len(desc) > 200: desc = desc[:200] + "..." - table.add_row("Description", desc or "[dim]No description[/]") + table.add_row("Description", _escape(desc) if desc else "[dim]No description[/]") table.add_row("Downloads", _format_count(info["downloads"])) table.add_row("Likes", _format_count(info["likes"])) table.add_row("Size", _format_size_bytes(info["size_bytes"])) if info["splits"]: splits_str = ", ".join( - f"{name} ({_format_count(count)})" + f"{_escape(str(name))} ({_format_count(count)})" for name, count in info["splits"].items() ) table.add_row("Splits", splits_str) @@ -1160,10 +1176,10 @@ def preview_dataset( table.add_row("Splits", "[dim]Not available (use streaming to explore)[/]") if info["features"]: - table.add_row("Features", ", ".join(info["features"])) + table.add_row("Features", ", ".join(_escape(str(f)) for f in info["features"])) if info["tags"]: - table.add_row("Tags", ", ".join(info["tags"][:10])) + table.add_row("Tags", ", ".join(_escape(str(t)) for t in info["tags"][:10])) console.print(table) diff --git a/src/soup_cli/commands/infer.py b/src/soup_cli/commands/infer.py index 62c31e8..0d5b689 100644 --- a/src/soup_cli/commands/infer.py +++ b/src/soup_cli/commands/infer.py @@ -164,6 +164,15 @@ def infer( # v0.71.32 — ASR (Whisper) transcription branch. Diverts before the chat # model-resolution path; _infer_asr owns its own Whisper load + output. if task == "asr": + # Validate --asr-task up front: a typo would otherwise be passed to + # whisper.generate(task=...) and fail INSIDE every row (100k confusing + # per-row skips instead of one upfront rejection). + if asr_task is not None and asr_task not in ("transcribe", "translate"): + console.print( + f"[red]--asr-task must be 'transcribe' or 'translate', " + f"got {asr_task!r}.[/]" + ) + raise typer.Exit(2) _infer_asr( model=model, base=base, @@ -301,6 +310,19 @@ _ASR_TRANSCRIBER_OVERRIDE = None # vector (mirrors the project's 10k custom-eval / 1e6 HF-download caps). _MAX_ASR_ROWS: int = 100_000 +# C0 control bytes (keep tab/newline/CR) + DEL, stripped from dataset-derived +# strings before they reach the terminal. rich.markup.escape() neutralises Rich +# '[...]' tags but NOT raw ESC bytes, and a row's audio path / an exception +# carrying it is untrusted (title-bar / OSC-8 spoofing). Mirrors v0.71.27 +# data_doctor._for_terminal. +_CONTROL_STRIP_TABLE = {i: None for i in range(0x20) if i not in (0x09, 0x0A, 0x0D)} +_CONTROL_STRIP_TABLE[0x7F] = None + + +def _for_terminal(text: str) -> str: + """Strip C0/ESC/DEL control bytes from a dataset-derived string.""" + return str(text).translate(_CONTROL_STRIP_TABLE) + def _read_asr_rows(path: Path) -> list[dict]: """Read ASR rows ``{"audio": path[, "text": reference]}`` from JSONL. @@ -537,6 +559,8 @@ def _infer_asr( console.print(f"[red]{exc}[/]") raise typer.Exit(2) from exc + from rich.markup import escape as _escape + refs: list[str] = [] hyps: list[str] = [] out_lines: list[str] = [] @@ -548,20 +572,47 @@ def _infer_asr( hyp = transcribe(resolved) except (ValueError, OSError, ImportError) as exc: skipped += 1 - console.print(f"[yellow]Skipped {Path(str(audio)).name!r}: {exc}[/]") + # Escape + control-strip the dataset-derived filename AND the + # exception (whose message embeds that filename) before printing. + name = _escape(_for_terminal(Path(str(audio)).name)) + console.print( + f"[yellow]Skipped {name!r}: {_escape(_for_terminal(str(exc)))}[/]" + ) continue rec = {"audio": audio, "transcription": hyp} ref = row.get("text") if isinstance(ref, str): - rec["reference"] = ref - rec["wer"] = wer(ref, hyp) - rec["cer"] = cer(ref, hyp) - refs.append(ref) - hyps.append(hyp) + # WER/CER can raise ValueError (the _MAX_RAW_CHARS DoS guard) on an + # oversized reference. Keep it INSIDE a try so one hostile row is + # skipped-unscored, not an uncaught crash that loses every already + # transcribed row's output (transcription cost is already paid). + try: + row_wer = wer(ref, hyp) + row_cer = cer(ref, hyp) + except ValueError as exc: + name = _escape(_for_terminal(Path(str(audio)).name)) + console.print( + f"[yellow]Metric skipped for {name!r}: " + f"{_escape(_for_terminal(str(exc)))}[/]" + ) + else: + rec["reference"] = ref + rec["wer"] = row_wer + rec["cer"] = row_cer + refs.append(ref) + hyps.append(hyp) out_lines.append(json.dumps(rec, ensure_ascii=False)) - atomic_write_text("\n".join(out_lines) + ("\n" if out_lines else ""), output_file, - field="--output") + # All rows failed to transcribe — do not write an empty file and claim + # success; a scripted pipeline (soup ship, CI) must see a non-zero exit. + if not out_lines: + console.print( + f"[red]No clips transcribed ({skipped} skipped). " + "Check --audio-dir and the input audio paths.[/]" + ) + raise typer.Exit(2) + + atomic_write_text("\n".join(out_lines) + "\n", output_file, field="--output") summary = f"Transcribed [bold]{len(out_lines)}[/] clip(s) -> {output_file}" if skipped: diff --git a/src/soup_cli/commands/runs.py b/src/soup_cli/commands/runs.py index 4ad2bc2..29feaeb 100644 --- a/src/soup_cli/commands/runs.py +++ b/src/soup_cli/commands/runs.py @@ -119,11 +119,14 @@ def list_runs( # Shorten run_id for display short_id = run["run_id"] + # experiment_name / base_model / task are config-derived — escape so a + # crafted value can't inject Rich markup (registry.py escapes the same + # fields). table.add_row( - short_id, - run.get("experiment_name") or "", - run.get("base_model") or "", - run.get("task") or "", + markup_escape(str(short_id)), + markup_escape(str(run.get("experiment_name") or "")), + markup_escape(str(run.get("base_model") or "")), + markup_escape(str(run.get("task") or "")), status_str, loss_str, str(run.get("total_steps") or ""), @@ -167,23 +170,25 @@ def show( minutes = int((secs % 3600) // 60) duration_str = f"{hours}h {minutes}m" if hours > 0 else f"{minutes}m" - # Build info panel + # Build info panel — config-derived fields escaped (mirrors registry.py). + _esc = markup_escape info_lines = [ - f"Run ID: [bold]{run['run_id']}[/]", - f"Name: {run.get('experiment_name') or '-'}", + f"Run ID: [bold]{_esc(str(run['run_id']))}[/]", + f"Name: {_esc(str(run.get('experiment_name') or '-'))}", f"Status: {status_str}", f"Date: {run['created_at'][:19].replace('T', ' ')}", "", - f"Model: [bold]{run.get('base_model') or '-'}[/]", - f"Task: {run.get('task') or '-'}", - f"Device: {run.get('device_name') or '-'} ({run.get('device') or '-'})", - f"GPU Memory: {run.get('gpu_memory') or '-'}", + f"Model: [bold]{_esc(str(run.get('base_model') or '-'))}[/]", + f"Task: {_esc(str(run.get('task') or '-'))}", + f"Device: {_esc(str(run.get('device_name') or '-'))} " + f"({_esc(str(run.get('device') or '-'))})", + f"GPU Memory: {_esc(str(run.get('gpu_memory') or '-'))}", "", f"Loss: {_fmt_loss(run)}", f"Steps: {run.get('total_steps') or '-'}", f"Duration: {duration_str}", f"Cost: {_fmt_cost(run)}", - f"Output: {run.get('output_dir') or '-'}", + f"Output: {_esc(str(run.get('output_dir') or '-'))}", ] console.print(Panel("\n".join(info_lines), title="Run Details")) diff --git a/src/soup_cli/commands/serve.py b/src/soup_cli/commands/serve.py index 2db2e68..505cf3e 100644 --- a/src/soup_cli/commands/serve.py +++ b/src/soup_cli/commands/serve.py @@ -1,8 +1,10 @@ """soup serve — local inference server with OpenAI-compatible API.""" +import contextlib import json import logging import re +import threading import time import uuid from collections.abc import Mapping @@ -722,6 +724,25 @@ def serve( console.print(f"[red]{exc}[/]") raise typer.Exit(1) + # v0.36.0 Part B: --trust-remote-code default-deny, resolved ONCE for + # every backend. vLLM previously loaded with an unconditional + # trust_remote_code=True (arbitrary repo code, zero notice) — resolve the + # same gate + warning panel the transformers path uses so no backend + # silently executes an untrusted repo's code. + from soup_cli.utils.trust_remote import ( + model_requires_trust_remote_code, + resolve_trust_remote_code, + ) + + _trust_probe_target = base_model or str(model_path) + _trust_requires = model_requires_trust_remote_code(str(model_path)) or False + resolved_trust = resolve_trust_remote_code( + _trust_probe_target, + requested=trust_remote_code, + console=console, + requires_remote_code=_trust_requires, + ) + if backend == "vllm": if speculative_model: console.print( @@ -741,6 +762,7 @@ def serve( num_speculative_tokens=num_speculative_tokens, enable_prefix_caching=prefix_cache, quantization=auto_quant_kwargs.get("quantization"), + trust_remote_code=resolved_trust, ) elif backend == "sglang": app = _serve_sglang( @@ -752,21 +774,8 @@ def serve( gpu_memory_utilization=gpu_memory_utilization, ) else: - # Transformers backend (original). - # v0.36.0 Part B: --trust-remote-code default-deny. - from soup_cli.utils.trust_remote import ( - model_requires_trust_remote_code, - resolve_trust_remote_code, - ) - - probe_target = base_model or str(model_path) - requires = model_requires_trust_remote_code(str(model_path)) or False - resolved_trust = resolve_trust_remote_code( - probe_target, - requested=trust_remote_code, - console=console, - requires_remote_code=requires, - ) + # Transformers backend (original). ``resolved_trust`` was computed + # once above (v0.36.0 Part B default-deny) and shared across backends. # v0.71.17 #259 — serve-time MoLE loads its OWN base + N task LoRAs + # gate; the `model` CLI arg is the base, the manifest supplies adapters @@ -815,6 +824,27 @@ def serve( f"— {resolved_kv_runtime.note}" ) + # v0.71.33 — actually load the --adapters map into the model so + # /v1/adapters/activate + the per-request `adapter` field switch the + # served weights (previously validated + tracked but never applied). + peft_adapter_names: set = set() + if adapter_map: + from rich.markup import escape as _esc + + try: + model_obj, peft_adapter_names = _load_named_adapters( + model_obj, adapter_map + ) + except Exception as exc: # noqa: BLE001 — surface any PEFT error + console.print( + f"[red]Failed to load --adapters:[/] {_esc(str(exc))}" + ) + raise typer.Exit(1) from exc + console.print( + "[green]Adapters ready:[/] " + + ", ".join(sorted(peft_adapter_names)) + ) + # v0.71.10 #201 — install the activation-steering decode hook. The # handle persists for the server's lifetime (process-global model). if steer is not None: @@ -986,6 +1016,7 @@ def serve( draft_model=draft_model, num_speculative_tokens=num_speculative_tokens, adapter_map=adapter_map if adapter_map else None, + peft_adapter_names=peft_adapter_names, output_constraint=constraint, enable_dashboard=dashboard, tracer=tracer, @@ -1035,6 +1066,7 @@ def _serve_vllm( num_speculative_tokens: int = 5, enable_prefix_caching: bool = False, quantization: Optional[str] = None, + trust_remote_code: bool = False, ): """Set up vLLM engine and create FastAPI app.""" from soup_cli.utils.vllm import create_vllm_app, create_vllm_engine @@ -1050,6 +1082,7 @@ def _serve_vllm( num_speculative_tokens=num_speculative_tokens, enable_prefix_caching=enable_prefix_caching, quantization=quantization, + trust_remote_code=trust_remote_code, ) console.print("[bold green]vLLM engine ready![/]") @@ -1172,6 +1205,67 @@ def _load_model( return model_obj, tokenizer +def _load_named_adapters(model_obj, adapter_map: Dict[str, str]): + """Load the ``--adapters name=path`` map into ``model_obj`` for hot-swap. + + Returns ``(model_obj, adapter_names)``. The returned model is a PeftModel + carrying every named adapter; ``adapter_names`` is the set actually loaded. + Request-time selection is done by :func:`_adapter_scope`. + + Without this, ``--adapters`` / ``POST /v1/adapters/activate`` / the + per-request ``adapter`` field were validated + tracked but NEVER applied — + every request silently ran the startup model (v0.71.33 fix). + """ + from peft import PeftModel + + names = list(adapter_map) + already_peft = isinstance(model_obj, PeftModel) + for idx, name in enumerate(names): + path = adapter_map[name] + if idx == 0 and not already_peft: + # Wrap the plain base model into a multi-adapter PeftModel. + model_obj = PeftModel.from_pretrained( + model_obj, path, adapter_name=name + ) + else: + model_obj.load_adapter(path, adapter_name=name) + console.print(f"[dim]Loaded adapter '{name}' from {path}[/]") + model_obj.eval() + return model_obj, set(names) + + +@contextlib.contextmanager +def _adapter_scope(model, lock, names, requested, active): + """Select the LoRA adapter for one generation, serialized by ``lock``. + + ``requested`` (request body ``adapter`` field) overrides ``active`` (the + ``/v1/adapters/activate`` selection). A name not in ``names`` (or ``None``) + runs the base model with adapters disabled. No-op when no named adapters + were loaded (``names`` empty), so the ordinary single-model serve path is + completely unaffected. + + The lock spans the whole generation because the PeftModel is process-global + and ``set_adapter`` mutates shared state — two concurrent requests on + different adapters would otherwise race. Generation is one blocking call in + every path (chat / stream / completions all generate-then-return), so this + serializes adapter-selected requests but never holds across true streaming. + """ + if not names or lock is None: + yield + return + name = requested or active + with lock: + if name and name in names and hasattr(model, "set_adapter"): + model.set_adapter(name) + yield + elif hasattr(model, "disable_adapter"): + # No (or unknown) adapter selected → base model for this request. + with model.disable_adapter(): + yield + else: + yield + + def _load_draft_model(speculative_model: str, device: str): """Load a smaller draft model for speculative decoding.""" import re @@ -1313,6 +1407,7 @@ def _create_app( draft_model=None, num_speculative_tokens: int = 5, adapter_map: Optional[Dict[str, str]] = None, + peft_adapter_names: Optional[set] = None, output_constraint: Optional[Dict] = None, enable_dashboard: bool = False, tracer=None, @@ -1398,6 +1493,10 @@ def _create_app( # Resolved adapter map (name → path) _adapter_map = adapter_map or {} + # v0.71.33 — adapters actually loaded into the PeftModel + the lock that + # serializes set_adapter + generate (the model is process-global). + _peft_adapter_names = peft_adapter_names or set() + _generation_lock = threading.Lock() # v0.71.12 #221 — VeRA / VB-LoRA bank loaded at startup (or None). The # active user is selected per request via the X-User-Id header. @@ -1520,6 +1619,10 @@ def _create_app( mole_runtime=_mole_runtime, loaded_bank=_loaded_bank, x_user_id=x_user_id, + adapter_lock=_generation_lock, + adapter_names=_peft_adapter_names, + requested_adapter=requested_adapter, + active_adapter=_active_snapshot(), ), media_type="text/event-stream", ) @@ -1558,17 +1661,28 @@ def _create_app( top_p=request.top_p, ) else: - response_text, prompt_tokens, completion_tokens = _generate_response( - model_obj, tokenizer, messages, - max_tokens=max_tokens, - temperature=request.temperature, - top_p=request.top_p, - assistant_model=draft_model, - num_assistant_tokens=num_speculative_tokens, - logits_processor=processors or None, - ngram_config=ngram_config, - kv_cache_generate_kwargs=kv_cache_generate_kwargs, - ) + # v0.71.33 — select the request's LoRA adapter (base = + # disabled) under the generation lock for the duration + # of generate(). + with _adapter_scope( + model_obj, _generation_lock, _peft_adapter_names, + requested_adapter, _active_snapshot(), + ): + ( + response_text, + prompt_tokens, + completion_tokens, + ) = _generate_response( + model_obj, tokenizer, messages, + max_tokens=max_tokens, + temperature=request.temperature, + top_p=request.top_p, + assistant_model=draft_model, + num_assistant_tokens=num_speculative_tokens, + logits_processor=processors or None, + ngram_config=ngram_config, + kv_cache_generate_kwargs=kv_cache_generate_kwargs, + ) except Exception: logger.exception("Generation error") raise HTTPException(status_code=500, detail="Internal server error") @@ -1975,6 +2089,8 @@ def _stream_response( kv_cache_generate_kwargs=None, mole_runtime=None, loaded_bank=None, x_user_id=None, + adapter_lock=None, adapter_names=None, + requested_adapter=None, active_adapter=None, ): """Generator that yields SSE chunks for streaming responses.""" chat_id = f"chatcmpl-{uuid.uuid4().hex[:8]}" @@ -2002,15 +2118,21 @@ def _stream_response( top_p=top_p, ) else: - response_text, _, completion_tokens_for_log = _generate_response( - model, tokenizer, messages, - max_tokens=max_tokens, - temperature=temperature, - top_p=top_p, - assistant_model=assistant_model, - num_assistant_tokens=num_assistant_tokens, - kv_cache_generate_kwargs=kv_cache_generate_kwargs, - ) + # v0.71.33 — select the request's LoRA adapter under the generation + # lock (resolved in the endpoint, applied here where generate runs). + with _adapter_scope( + model, adapter_lock, adapter_names, + requested_adapter, active_adapter, + ): + response_text, _, completion_tokens_for_log = _generate_response( + model, tokenizer, messages, + max_tokens=max_tokens, + temperature=temperature, + top_p=top_p, + assistant_model=assistant_model, + num_assistant_tokens=num_assistant_tokens, + kv_cache_generate_kwargs=kv_cache_generate_kwargs, + ) except Exception: logger.exception("Stream generation error") yield 'data: {"error": "Internal server error"}\n\n' diff --git a/src/soup_cli/commands/train.py b/src/soup_cli/commands/train.py index 46ce50c..d9a2121 100644 --- a/src/soup_cli/commands/train.py +++ b/src/soup_cli/commands/train.py @@ -785,7 +785,16 @@ def train( is_in_distributed, ) - if not is_in_distributed(): + if dry_run and not is_in_distributed(): + # --dry-run must NEVER os.execvp into a real multi-GPU run. + # Without this guard the re-exec fired before the dry_run check + # (~350 lines below), so `soup train --dry-run --gpus N` launched + # a full accelerate run instead of just validating. + console.print( + f"[dim]--dry-run: skipping accelerate re-exec " + f"({num_gpus} GPUs, {topo['interconnect']}).[/]" + ) + elif not is_in_distributed(): # v0.33.0 #37 — auto-reexec under accelerate launch unless # --no-reexec was passed. Reexec uses os.execvp so the new # accelerate process replaces this process; no leftover PID @@ -869,6 +878,15 @@ def train( script_args.extend(["--energy-out", energy_out]) if yes: script_args.append("--yes") + # Distillation / activation-capture flags — same drop-on-reexec + # bug class as the block above: a multi-GPU run silently ignored + # them (MiniLLM stayed offline, no activation snapshot written). + if minillm_on_policy: + script_args.append("--minillm-on-policy") + if capture_activations: + script_args.extend(["--capture-activations", capture_activations]) + if capture_prompts: + script_args.extend(["--capture-prompts", capture_prompts]) argv = build_accelerate_argv( num_processes=num_gpus, script_args=script_args, ) @@ -890,22 +908,25 @@ def train( "the launch command for manual execution." ) raise typer.Exit(1) from exc - console.print( - f"[green]Distributed run detected[/] " - f"({num_gpus} procs, {topo['interconnect']} interconnect)" - ) - # Apply NCCL env hints. All current keys (``NCCL_P2P_DISABLE`` / - # ``NCCL_IB_DISABLE`` / ``NCCL_NVLS_ENABLE``) are rank-idempotent - # string literals so it is safe to run on every rank. If a - # rank-sensitive key is ever added to ``suggest_nccl_env``, this - # loop must be gated to ``LOCAL_RANK == 0``. ``setdefault`` keeps - # user / launcher overrides winning over our suggestions. - from soup_cli.utils.topology import suggest_nccl_env + elif is_in_distributed(): + # Already a launched rank — announce + apply NCCL hints. (The + # dry_run branch above intentionally does neither.) + console.print( + f"[green]Distributed run detected[/] " + f"({num_gpus} procs, {topo['interconnect']} interconnect)" + ) + # Apply NCCL env hints. All current keys (``NCCL_P2P_DISABLE`` / + # ``NCCL_IB_DISABLE`` / ``NCCL_NVLS_ENABLE``) are rank-idempotent + # string literals so it is safe to run on every rank. If a + # rank-sensitive key is ever added to ``suggest_nccl_env``, this + # loop must be gated to ``LOCAL_RANK == 0``. ``setdefault`` keeps + # user / launcher overrides winning over our suggestions. + from soup_cli.utils.topology import suggest_nccl_env - for key, val in suggest_nccl_env( - gpu_count=num_gpus, interconnect=topo["interconnect"] - ).items(): - os.environ.setdefault(key, val) + for key, val in suggest_nccl_env( + gpu_count=num_gpus, interconnect=topo["interconnect"] + ).items(): + os.environ.setdefault(key, val) # Detect hardware device, device_name = detect_device() diff --git a/src/soup_cli/config/loader.py b/src/soup_cli/config/loader.py index 32567be..7a66ee5 100644 --- a/src/soup_cli/config/loader.py +++ b/src/soup_cli/config/loader.py @@ -41,6 +41,13 @@ def load_config_from_string(yaml_str: str) -> SoupConfig: raw = yaml.safe_load(yaml_str) if raw is None: raise ValueError("Config is empty") + if not isinstance(raw, dict): + # A non-mapping document (e.g. a bare list "- a") would make + # SoupConfig(**raw) raise TypeError, breaking this function's + # ValueError-only contract (API/UI callers only catch ValueError). + raise ValueError( + f"Config must be a YAML mapping, got {type(raw).__name__}" + ) try: return SoupConfig(**raw) diff --git a/src/soup_cli/data/loader.py b/src/soup_cli/data/loader.py index 83cce64..e9d73c1 100644 --- a/src/soup_cli/data/loader.py +++ b/src/soup_cli/data/loader.py @@ -170,11 +170,17 @@ def load_dataset(data_config: DataConfig) -> dict: def _validate_vision_images(data: list[dict], image_dir: Path) -> list[dict]: """Validate and resolve image paths in vision dataset rows. - Each row must have an 'image' key with a filename or path. - Resolves relative paths against image_dir. + Each row must have an 'image' key with a filename or path. Resolves + relative paths against image_dir and rejects path traversal — a crafted + llava/sharegpt4v row like ``{"image": "/etc/passwd"}`` must not be handed + to ``PIL.Image.open``. Mirrors :func:`_validate_audio_files` (the sibling + audio path got this fix in v0.71.32; the vision path was missed). """ + from soup_cli.utils.paths import is_under + valid = [] missing = 0 + traversal = 0 for row in data: if "image" not in row or not row["image"]: missing += 1 @@ -182,11 +188,21 @@ def _validate_vision_images(data: list[dict], image_dir: Path) -> list[dict]: image_path = Path(row["image"]) if not image_path.is_absolute(): image_path = image_dir / image_path - row["image"] = str(image_path) - valid.append(row) + # Path traversal protection: resolved path must stay under image_dir. + # realpath + commonpath (is_under) — Path.is_relative_to() breaks on + # Windows 8.3 short names. + if not is_under(image_path, image_dir): + traversal += 1 + continue + valid.append({**row, "image": str(image_path.resolve())}) if missing > 0: console.print(f"[yellow]Warning: {missing} rows skipped (missing image path)[/]") + if traversal > 0: + console.print( + f"[yellow]Warning: {traversal} rows skipped " + f"(image path outside {image_dir})[/]" + ) return valid diff --git a/src/soup_cli/trainer/asr.py b/src/soup_cli/trainer/asr.py index de5c262..dba3ada 100644 --- a/src/soup_cli/trainer/asr.py +++ b/src/soup_cli/trainer/asr.py @@ -257,13 +257,34 @@ class AsrTrainerWrapper: from soup_cli.utils.tts_codec import load_audio_mono + # Whisper's decoder is capped at max_target_positions (448 for every + # size). A transcript that tokenizes longer would crash mid-training + # with an opaque positional-embedding index error, so cap the labels — + # honouring a smaller data.max_length when the user set one (previously + # ignored). Audio > 30 s is silently truncated to 3000 mel frames by the + # feature extractor while the full transcript stays in the labels (that + # teaches hallucination), so count both and warn once after the map. + max_target_positions = int( + getattr(self._unwrapped_model().config, "max_target_positions", 448) + ) + label_cap = max_target_positions + if cfg.data.max_length and int(cfg.data.max_length) < label_cap: + label_cap = int(cfg.data.max_length) + max_audio_samples = _ASR_SAMPLE_RATE * 30 + trunc = {"labels": 0, "audio": 0} + def encode(row: dict) -> dict: audio_path, text = _validate_asr_row(row) wave = load_audio_mono(audio_path, target_sr=_ASR_SAMPLE_RATE) + if len(wave) > max_audio_samples: + trunc["audio"] += 1 features = feature_extractor( wave, sampling_rate=_ASR_SAMPLE_RATE ).input_features[0] labels = tokenizer(text).input_ids + if len(labels) > label_cap: + labels = labels[:label_cap] + trunc["labels"] += 1 return {"input_features": features, "labels": labels} raw_train = Dataset.from_list(dataset["train"]) @@ -272,6 +293,16 @@ class AsrTrainerWrapper: if dataset.get("val"): raw_val = Dataset.from_list(dataset["val"]) eval_ds = raw_val.map(encode, remove_columns=raw_val.column_names) + if trunc["labels"]: + console.print( + f"[yellow]{trunc['labels']} row(s) had transcripts longer than " + f"{label_cap} tokens; labels truncated (Whisper decoder limit).[/]" + ) + if trunc["audio"]: + console.print( + f"[yellow]{trunc['audio']} row(s) had audio >30s; the feature " + f"extractor truncates to 30s — the transcript may not align.[/]" + ) output_dir = Path(cfg.output) if cfg.experiment_name: @@ -285,6 +316,18 @@ class AsrTrainerWrapper: ) warmup_steps = int(total_steps * tcfg.warmup_ratio) + # Mixed precision by GPU capability — bf16=cuda was hardcoded, which + # crashes on pre-Ampere cards (T4 / GTX 16xx) that lack bf16. Fall back + # to fp16 there; fp32 on CPU. + use_bf16 = use_fp16 = False + if self.device == "cuda": + import torch + + if torch.cuda.is_bf16_supported(): + use_bf16 = True + else: + use_fp16 = True + args = Seq2SeqTrainingArguments( output_dir=str(output_dir), num_train_epochs=tcfg.epochs, @@ -299,7 +342,8 @@ class AsrTrainerWrapper: logging_steps=tcfg.logging_steps, save_steps=tcfg.save_steps, save_total_limit=3, - bf16=self.device == "cuda", + bf16=use_bf16, + fp16=use_fp16, report_to=self.report_to, deepspeed=self.deepspeed_config, predict_with_generate=True, diff --git a/src/soup_cli/trainer/distill.py b/src/soup_cli/trainer/distill.py index 7fb5c1f..cb5a55c 100644 --- a/src/soup_cli/trainer/distill.py +++ b/src/soup_cli/trainer/distill.py @@ -77,6 +77,21 @@ def _compute_distill_term( raise ValueError( f"temperature must be finite and positive, got {temperature!r}" ) + + # Causal-LM alignment: logits at position i predict token i+1, so the CE + # term shifts (logits[:, :-1] vs labels[:, 1:]). The KD term must shift the + # SAME way — otherwise the trained-token mask (labels != -100) is applied + # one position off: it drops each assistant span's first predicted token and + # leaks the boundary token just before the span. Shift here so both terms + # measure the same positions. + if labels is not None or attention_mask is not None: + student_logits = student_logits[:, :-1, :] + teacher_logits = teacher_logits[:, :-1, :] + if labels is not None: + labels = labels[:, 1:] + if attention_mask is not None: + attention_mask = attention_mask[:, 1:] + temp = float(temperature) s = student_logits / temp t = teacher_logits / temp diff --git a/src/soup_cli/trainer/mlx_sft.py b/src/soup_cli/trainer/mlx_sft.py index ff01e32..6d4488c 100644 --- a/src/soup_cli/trainer/mlx_sft.py +++ b/src/soup_cli/trainer/mlx_sft.py @@ -101,10 +101,17 @@ class MLXSFTTrainerWrapper: adapter_file=str(output_dir / "adapters.safetensors"), ) + # mlx_lm.tuner.trainer.train requires a real optimizer — passing None + # left the model untrained (AttributeError on optimizer.update, or a + # silent no-op checkpoint). Build an AdamW from the configured LR. + import mlx.optimizers as optim # type: ignore + + optimizer = optim.AdamW(learning_rate=float(cfg.training.lr)) + train( model=self.model, tokenizer=self.tokenizer, - optimizer=None, + optimizer=optimizer, train_dataset=self._dataset.get("train", []), val_dataset=self._dataset.get("val", []), training_callback=None, diff --git a/src/soup_cli/ui/app.py b/src/soup_cli/ui/app.py index 88c4eec..3ac1da0 100644 --- a/src/soup_cli/ui/app.py +++ b/src/soup_cli/ui/app.py @@ -114,7 +114,9 @@ def create_app(host: str = "127.0.0.1", port: int = 7860): auth = request.headers.get("Authorization", "") with _auth_token_lock: expected = f"Bearer {_auth_token}" - if auth != expected: + # Constant-time compare — a plain != leaks the token byte-by-byte via + # response timing when `soup ui --public` is exposed on a LAN. + if not secrets.compare_digest(auth, expected): raise HTTPException(status_code=401, detail="Unauthorized") # --- Static files --- @@ -504,7 +506,11 @@ def create_app(host: str = "127.0.0.1", port: int = 7860): run_id: Optional[str] = Query(default=None), ): """Return current training progress snapshot.""" - proc = _train_process + # Read the shared process handle under the lock, like every sibling + # endpoint (start/status/stop) — avoids a torn read racing a concurrent + # start/stop. + with _train_lock: + proc = _train_process is_running = proc is not None and proc.poll() is None if not is_running and run_id is None: diff --git a/src/soup_cli/utils/lr_finder.py b/src/soup_cli/utils/lr_finder.py index 80ce3a6..dab52b4 100644 --- a/src/soup_cli/utils/lr_finder.py +++ b/src/soup_cli/utils/lr_finder.py @@ -11,7 +11,7 @@ import math from pathlib import Path from typing import Optional, Sequence, TypedDict -from soup_cli.utils.paths import is_under_cwd +from soup_cli.utils.paths import atomic_write_text, is_under_cwd class LRFinderResult(TypedDict): @@ -217,11 +217,11 @@ def save_lr_finder_report( "min_loss_lr": summary["min_loss_lr"], "diverged_at": summary["diverged_at"], } - output.parent.mkdir(parents=True, exist_ok=True) # ``allow_nan=False`` is belt-and-braces: report_* are already finite, # but ``smoothed_losses`` could carry a non-finite if the input loss - # somehow drifted. Reject rather than emit ``NaN`` (invalid JSON). - output.write_text( - json.dumps(payload, indent=2, allow_nan=False), - encoding="utf-8", - ) + # somehow drifted. Reject (raises ValueError) rather than emit ``NaN`` + # (invalid JSON) — computed before the write so nothing partial lands. + body = json.dumps(payload, indent=2, allow_nan=False) + # Atomic write + symlink rejection (centralised TOCTOU defence) instead of + # a plain write_text with no symlink check. + atomic_write_text(body, str(output), field="output_path") diff --git a/src/soup_cli/utils/vllm.py b/src/soup_cli/utils/vllm.py index 87b8db9..ee76a67 100644 --- a/src/soup_cli/utils/vllm.py +++ b/src/soup_cli/utils/vllm.py @@ -42,6 +42,7 @@ def create_vllm_engine( enable_prefix_caching: bool = False, quantization: Optional[str] = None, sleep_mode: bool = False, + trust_remote_code: bool = False, ): """Create a vLLM AsyncLLMEngine for serving. @@ -57,6 +58,10 @@ def create_vllm_engine( win for RAG / agent workloads with shared system prompts. sleep_mode: Enable vLLM sleep/standby support (v0.71.21 #124 — requires vLLM >= 0.7; raises a friendly RuntimeError otherwise). + trust_remote_code: Execute custom repo code on model load. Default + False — the caller (``serve``) resolves this through the shared + v0.36.0 ``resolve_trust_remote_code`` gate (flag + warning panel) + so an untrusted HF repo never runs code silently. Returns: (engine, engine_model_name) tuple. @@ -72,7 +77,7 @@ def create_vllm_engine( tensor_parallel_size=tensor_parallel_size, gpu_memory_utilization=gpu_memory_utilization, dtype=dtype, - trust_remote_code=True, + trust_remote_code=trust_remote_code, enable_prefix_caching=enable_prefix_caching, ) if max_model_len is not None: @@ -84,7 +89,7 @@ def create_vllm_engine( tensor_parallel_size=tensor_parallel_size, gpu_memory_utilization=gpu_memory_utilization, dtype=dtype, - trust_remote_code=True, + trust_remote_code=trust_remote_code, enable_prefix_caching=enable_prefix_caching, ) if max_model_len is not None: diff --git a/src/soup_cli/utils/vscode_setup.py b/src/soup_cli/utils/vscode_setup.py index 91e40c1..3ecf555 100644 --- a/src/soup_cli/utils/vscode_setup.py +++ b/src/soup_cli/utils/vscode_setup.py @@ -9,9 +9,8 @@ from __future__ import annotations import json import os -import stat -from soup_cli.utils.paths import is_under_cwd +from soup_cli.utils.paths import atomic_write_text, is_under_cwd def build_launch_json(*, config_path: str = "soup.yaml") -> dict: @@ -81,24 +80,15 @@ def write_vscode_launch( payload = build_launch_json(config_path=config_path) os.makedirs(real, exist_ok=True) out_path = os.path.join(real, "launch.json") - # TOCTOU defence: reject symlinks at the target path regardless of - # `force` (mirrors v0.33.0 #22 prune_checkpoints policy). Without this - # guard, force=True would follow a pre-placed symlink and overwrite a - # file outside cwd. - try: - st = os.lstat(out_path) - except FileNotFoundError: - st = None - if st is not None: - if stat.S_ISLNK(st.st_mode): - raise ValueError( - "launch.json target is a symlink; aborting" - ) - if not force: - raise FileExistsError( - f"{out_path} already exists; pass force=True to overwrite" - ) - with open(out_path, "w", encoding="utf-8") as f: - json.dump(payload, f, indent=2, ensure_ascii=False) - f.write("\n") + # Refuse to clobber an existing non-symlink file unless force. A symlink at + # the target (regardless of force) is rejected by atomic_write_text's + # enforce_under_cwd_and_no_symlink, which also closes the lstat-then-open + # TOCTOU window: bytes go to a fresh mkstemp file + os.replace, never + # through a swapped-in symlink. + if not force and os.path.lexists(out_path) and not os.path.islink(out_path): + raise FileExistsError( + f"{out_path} already exists; pass force=True to overwrite" + ) + body = json.dumps(payload, indent=2, ensure_ascii=False) + "\n" + atomic_write_text(body, out_path, field="launch.json") return out_path diff --git a/tests/test_code_review_high.py b/tests/test_code_review_high.py index 46bd191..92a81d0 100644 --- a/tests/test_code_review_high.py +++ b/tests/test_code_review_high.py @@ -102,24 +102,39 @@ def test_beta_schedule_callback_sets_beta_from_start(): def test_distill_term_masks_padding_and_prompt(): - """The default distillation divergence must be measured only over trained - tokens (labels != -100), not padding/prompt.""" + """The default distillation divergence is measured only over trained tokens, + CAUSALLY ALIGNED with the CE term: logit position i is trained iff + labels[i+1] != -100 (v0.71.33 off-by-one fix — the mask was previously + applied to the unshifted labels, dropping each span's first predicted token + and leaking the boundary token past it).""" torch = pytest.importorskip("torch") from soup_cli.trainer.distill import _compute_distill_term student = torch.zeros(1, 3, 4) teacher = torch.zeros(1, 3, 4) - # Positions 0,1 diverge wildly; position 2 is identical (KL 0). - student[0, 0, 0] = 10.0 - teacher[0, 0, 3] = 10.0 + # Divergence localised at logit position 1, which predicts token index 2. student[0, 1, 1] = 10.0 teacher[0, 1, 2] = 10.0 - labels = torch.tensor([[-100, -100, 5]]) # only the last token is trained - masked = float(_compute_distill_term(student, teacher, "forward_kl", 1.0, labels=labels)) + # Token index 2 is the only trained target → logit position 1 is trained → + # the divergence there IS measured. + labels_trained = torch.tensor([[-100, -100, 5]]) + masked = float( + _compute_distill_term(student, teacher, "forward_kl", 1.0, labels=labels_trained) + ) + assert masked > 0.1, masked + + # Token index 1 trained instead → logit position 0 is the trained one, but + # the divergence lives at position 1 → excluded → ~0. + labels_other = torch.tensor([[-100, 5, -100]]) + masked_other = float( + _compute_distill_term(student, teacher, "forward_kl", 1.0, labels=labels_other) + ) + assert masked_other < 1e-5, masked_other + + # Unmasked averages over every position → the divergent one still shows. unmasked = float(_compute_distill_term(student, teacher, "forward_kl", 1.0)) - assert masked < 1e-5, masked # only the identical (trained) position counts - assert unmasked > 0.1, unmasked # divergent prompt positions dominate unmasked + assert unmasked > 0.05, unmasked def test_kto_negative_one_label_is_undesirable(): diff --git a/tests/test_review_fixes_v07133.py b/tests/test_review_fixes_v07133.py new file mode 100644 index 0000000..3e86923 --- /dev/null +++ b/tests/test_review_fixes_v07133.py @@ -0,0 +1,662 @@ +"""Regression tests for the v0.71.32 full-tree review findings. + +Covers the 6 HIGH + MEDIUM/LOW findings surfaced by the review round: +vLLM trust_remote_code gate, vision image path containment, functional +multi-adapter serving, --dry-run/--gpus re-exec guard + dropped-flag forwarding, +MLX optimizer construction, Rich-markup escaping in `soup data`, the ASR infer +metric-guard / control-strip / task-validation / exit-code fixes, the distill KD +off-by-one shift, the load_config_from_string ValueError contract, `soup runs` +escaping, the constant-time UI token compare, and the atomic-write centralisation. +""" + +from __future__ import annotations + +import json +import sys +import types +from pathlib import Path + +import pytest +from typer.testing import CliRunner + + +# -------------------------------------------------------------------------- +# HIGH-1 — vLLM serve must not force trust_remote_code=True +# -------------------------------------------------------------------------- +class TestVllmTrustRemoteCode: + def _install_fake_vllm(self, monkeypatch): + captured: dict = {} + + class _FakeEngineArgs: + def __init__(self, **kwargs): + captured.update(kwargs) + + class _FakeEngine: + @classmethod + def from_engine_args(cls, args): + return object() + + fake = types.ModuleType("vllm") + fake.AsyncEngineArgs = _FakeEngineArgs + fake.AsyncLLMEngine = _FakeEngine + monkeypatch.setitem(sys.modules, "vllm", fake) + return captured + + def test_default_is_false(self, monkeypatch): + from soup_cli.utils.vllm import create_vllm_engine + + captured = self._install_fake_vllm(monkeypatch) + create_vllm_engine(model_path="some/model") + assert captured["trust_remote_code"] is False + + def test_true_is_threaded_through(self, monkeypatch): + from soup_cli.utils.vllm import create_vllm_engine + + captured = self._install_fake_vllm(monkeypatch) + create_vllm_engine(model_path="some/model", trust_remote_code=True) + assert captured["trust_remote_code"] is True + + def test_serve_vllm_passes_resolved_trust(self, monkeypatch): + # _serve_vllm must forward its trust_remote_code down to the engine. + import soup_cli.commands.serve as serve + + seen: dict = {} + + def _fake_engine(**kwargs): + seen.update(kwargs) + return object(), "m" + + monkeypatch.setattr( + "soup_cli.utils.vllm.create_vllm_engine", _fake_engine + ) + monkeypatch.setattr( + "soup_cli.utils.vllm.create_vllm_app", + lambda **kw: object(), + ) + serve._serve_vllm( + model_path=Path("m"), + base_model=None, + is_adapter=False, + max_tokens_default=128, + tensor_parallel=1, + gpu_memory_utilization=0.9, + trust_remote_code=True, + ) + assert seen.get("trust_remote_code") is True + + +# -------------------------------------------------------------------------- +# HIGH-2 — vision dataset image paths must be containment-checked +# -------------------------------------------------------------------------- +class TestVisionImageContainment: + def test_absolute_traversal_dropped(self, tmp_path): + from soup_cli.data.loader import _validate_vision_images + + img_dir = tmp_path / "imgs" + img_dir.mkdir() + (img_dir / "ok.png").write_bytes(b"x") + rows = [ + {"image": "ok.png", "messages": []}, + {"image": "/etc/passwd", "messages": []}, + ] + out = _validate_vision_images([dict(r) for r in rows], img_dir) + kept = [Path(r["image"]).name for r in out] + assert "ok.png" in kept + assert "passwd" not in kept + assert len(out) == 1 + + def test_relative_traversal_dropped(self, tmp_path): + from soup_cli.data.loader import _validate_vision_images + + img_dir = tmp_path / "imgs" + img_dir.mkdir() + secret = tmp_path / "secret.png" + secret.write_bytes(b"x") + out = _validate_vision_images( + [{"image": "../secret.png", "messages": []}], img_dir + ) + assert out == [] + + +# -------------------------------------------------------------------------- +# HIGH-3 — multi-adapter serving actually switches adapters +# -------------------------------------------------------------------------- +class _FakeAdapterModel: + """Records set_adapter + disable_adapter usage.""" + + def __init__(self): + self.set_calls: list = [] + self.disable_calls = 0 + self.active = None + + def set_adapter(self, name): + self.set_calls.append(name) + self.active = name + + def disable_adapter(self): + model = self + + class _Ctx: + def __enter__(self): + model.disable_calls += 1 + return self + + def __exit__(self, *exc): + return False + + return _Ctx() + + +class TestAdapterScope: + def _run(self, requested, active, names): + import threading + + from soup_cli.commands.serve import _adapter_scope + + model = _FakeAdapterModel() + lock = threading.Lock() + with _adapter_scope(model, lock, names, requested, active): + pass + return model + + def test_requested_overrides_active(self): + model = self._run("code", "chat", {"chat", "code"}) + assert model.set_calls == ["code"] + assert model.disable_calls == 0 + + def test_falls_back_to_active(self): + model = self._run(None, "chat", {"chat", "code"}) + assert model.set_calls == ["chat"] + + def test_unknown_name_uses_base(self): + model = self._run("nope", None, {"chat"}) + assert model.set_calls == [] + assert model.disable_calls == 1 + + def test_none_selects_base(self): + model = self._run(None, None, {"chat"}) + assert model.disable_calls == 1 + + def test_no_adapters_is_noop(self): + model = self._run("chat", "chat", set()) + assert model.set_calls == [] + assert model.disable_calls == 0 + + +class TestLoadNamedAdapters: + def test_wraps_plain_model_then_loads_rest(self, monkeypatch): + import soup_cli.commands.serve as serve + + loaded: dict = {"from_pretrained": [], "load_adapter": []} + + class _FakePeft: + def __init__(self, name="default"): + self.name = name + + @classmethod + def from_pretrained(cls, model, path, adapter_name=None): + loaded["from_pretrained"].append((path, adapter_name)) + return cls(adapter_name) + + def load_adapter(self, path, adapter_name=None): + loaded["load_adapter"].append((path, adapter_name)) + + def eval(self): + return self + + fake_peft = types.ModuleType("peft") + fake_peft.PeftModel = _FakePeft + monkeypatch.setitem(sys.modules, "peft", fake_peft) + + plain = object() # not a PeftModel → must be wrapped + model, names = serve._load_named_adapters( + plain, {"chat": "a/path", "code": "b/path"} + ) + assert names == {"chat", "code"} + # First adapter wraps the base; the rest are load_adapter'd. + assert loaded["from_pretrained"] == [("a/path", "chat")] + assert loaded["load_adapter"] == [("b/path", "code")] + + +# -------------------------------------------------------------------------- +# HIGH-4 — --dry-run must NOT re-exec, and dropped flags must forward +# -------------------------------------------------------------------------- +def _write_sft_config(tmp_path): + (tmp_path / "soup.yaml").write_text( + "base: test/model\n" + "task: sft\n" + "data: {train: data.jsonl, format: alpaca}\n" + "training: {epochs: 1, lr: 1e-4, batch_size: 1}\n", + encoding="utf-8", + ) + + +def _force_two_gpus(monkeypatch): + from soup_cli.commands import train as train_cmd + from soup_cli.utils import launcher as launcher_mod + from soup_cli.utils import topology as topo_mod + + for var in ("RANK", "WORLD_SIZE", "LOCAL_RANK"): + monkeypatch.delenv(var, raising=False) + topo = {"gpu_count": 2, "interconnect": "PCIe"} + monkeypatch.setattr(topo_mod, "detect_topology", lambda: topo) + monkeypatch.setattr(topo_mod, "resolve_num_gpus", lambda spec: 2) + monkeypatch.setattr(train_cmd, "detect_topology", lambda: topo, raising=False) + monkeypatch.setattr( + train_cmd, "resolve_num_gpus", lambda spec: 2, raising=False + ) + monkeypatch.setattr(launcher_mod, "is_in_distributed", lambda: False) + + +class TestDryRunNoReexec: + def test_dry_run_does_not_execvp(self, tmp_path, monkeypatch): + from soup_cli.cli import app + + monkeypatch.chdir(tmp_path) + _write_sft_config(tmp_path) + _force_two_gpus(monkeypatch) + + called = {"execvp": False} + + def _fake_execvp(file, argv): + called["execvp"] = True + raise SystemExit(99) + + monkeypatch.setattr("os.execvp", _fake_execvp) + + CliRunner().invoke( + app, ["train", "--config", "soup.yaml", "--gpus", "2", "--dry-run"] + ) + assert called["execvp"] is False, ( + "os.execvp was called on a --dry-run multi-GPU invocation" + ) + + +class TestReexecForwardsFlags: + def test_capture_flags_forwarded(self, tmp_path, monkeypatch): + from soup_cli.cli import app + + monkeypatch.chdir(tmp_path) + _write_sft_config(tmp_path) + _force_two_gpus(monkeypatch) + + captured: dict = {} + + def _fake_execvp(file, argv): + captured["argv"] = list(argv) + raise SystemExit(99) + + monkeypatch.setattr("os.execvp", _fake_execvp) + + CliRunner().invoke( + app, + [ + "train", "--config", "soup.yaml", "--gpus", "2", "--yes", + "--capture-activations", "model.layers.5", + "--capture-prompts", "p.jsonl", + ], + ) + argv = captured.get("argv", []) + assert "--capture-activations" in argv + assert "model.layers.5" in argv + assert "--capture-prompts" in argv + assert "p.jsonl" in argv + + +# -------------------------------------------------------------------------- +# HIGH-5 — MLX SFT must build a real optimizer (not pass None) +# -------------------------------------------------------------------------- +class TestMlxOptimizer: + def test_train_passes_non_none_optimizer(self, tmp_path, monkeypatch): + from soup_cli.config.loader import load_config_from_string + from soup_cli.trainer.mlx_sft import MLXSFTTrainerWrapper + + cfg = load_config_from_string( + "base: mlx-community/tiny\n" + "task: sft\n" + "backend: mlx\n" + "data: {train: d.jsonl, format: chatml}\n" + "training: {epochs: 1, lr: 2e-4, batch_size: 1}\n" + f"output: {json.dumps(str(tmp_path / 'out'))}\n" + ) + wrapper = MLXSFTTrainerWrapper(cfg) + wrapper.model = object() + wrapper.tokenizer = object() + wrapper._dataset = {"train": [{"messages": []}], "val": []} + monkeypatch.setattr(wrapper, "_require_mlx", lambda: None) + + seen: dict = {} + + def _fake_train(**kwargs): + seen.update(kwargs) + + class _FakeArgs: + def __init__(self, **kwargs): + pass + + sentinel = object() + + trainer_mod = types.ModuleType("mlx_lm.tuner.trainer") + trainer_mod.TrainingArgs = _FakeArgs + trainer_mod.train = _fake_train + tuner_mod = types.ModuleType("mlx_lm.tuner") + mlx_lm_mod = types.ModuleType("mlx_lm") + opt_mod = types.ModuleType("mlx.optimizers") + opt_mod.AdamW = lambda learning_rate=None: sentinel + mlx_root = types.ModuleType("mlx") + + monkeypatch.setitem(sys.modules, "mlx", mlx_root) + monkeypatch.setitem(sys.modules, "mlx.optimizers", opt_mod) + monkeypatch.setitem(sys.modules, "mlx_lm", mlx_lm_mod) + monkeypatch.setitem(sys.modules, "mlx_lm.tuner", tuner_mod) + monkeypatch.setitem(sys.modules, "mlx_lm.tuner.trainer", trainer_mod) + + wrapper.train() + assert "optimizer" in seen + assert seen["optimizer"] is sentinel + + +# -------------------------------------------------------------------------- +# HIGH-6 — `soup data inspect` must escape dataset-derived Rich markup +# -------------------------------------------------------------------------- +class TestDataInspectEscape: + def test_bracket_slash_does_not_crash(self, tmp_path, monkeypatch): + from soup_cli.commands.data import app as data_app + + monkeypatch.chdir(tmp_path) + data_file = tmp_path / "d.jsonl" + # A stray '[/]' + a crafted link tag in ordinary data. + rows = [ + {"instruction": "hi [/] there", "output": "[link=http://x]clickme[/]"}, + {"instruction": "normal", "output": "fine"}, + ] + data_file.write_text( + "\n".join(json.dumps(r) for r in rows), encoding="utf-8" + ) + result = CliRunner().invoke(data_app, ["inspect", "d.jsonl"]) + assert result.exit_code == 0, (result.output, repr(result.exception)) + # Literal markup survives as text (escaped), not interpreted. + assert "[/]" in result.output + + +# -------------------------------------------------------------------------- +# ASR — infer.py metric guard / control-strip / task-validate / exit code +# -------------------------------------------------------------------------- +def _write_asr_input(tmp_path, rows): + p = tmp_path / "in.jsonl" + p.write_text("\n".join(json.dumps(r) for r in rows), encoding="utf-8") + return p + + +class TestAsrInferMetricGuard: + def test_oversized_reference_does_not_crash(self, tmp_path, monkeypatch): + import soup_cli.commands.infer as infer + from soup_cli.cli import app as cli_app + + monkeypatch.chdir(tmp_path) + (tmp_path / "a.wav").write_bytes(b"x") + (tmp_path / "b.wav").write_bytes(b"x") + big = "word " * 300_000 # > _MAX_RAW_CHARS + _write_asr_input( + tmp_path, + [ + {"audio": "a.wav", "text": big}, # metric must be skipped + {"audio": "b.wav", "text": "hello"}, # metric ok + ], + ) + monkeypatch.setattr(infer, "_ASR_TRANSCRIBER_OVERRIDE", lambda p: "hello") + + result = CliRunner().invoke( + cli_app, + ["infer", "--task", "asr", "--model", "whatever", + "--input", "in.jsonl", "--output", "out.jsonl"], + ) + assert result.exit_code == 0, (result.output, repr(result.exception)) + lines = [ + json.loads(x) + for x in (tmp_path / "out.jsonl").read_text().splitlines() if x + ] + assert len(lines) == 2 # both rows still transcribed + # Oversized row present but unscored; the small row scored. + assert "wer" not in lines[0] + assert "wer" in lines[1] + + +class TestAsrSkipControlStrip: + def test_hostile_filename_is_stripped(self, tmp_path, monkeypatch): + import soup_cli.commands.infer as infer + from soup_cli.cli import app as cli_app + + monkeypatch.chdir(tmp_path) + # A filename carrying a raw ESC byte; transcriber raises so the skip + # warning path (which prints the name) runs. + (tmp_path / "clip.wav").write_bytes(b"x") + + def _boom(_p): + raise ValueError("bad \x1b[31maudio") + + monkeypatch.setattr(infer, "_ASR_TRANSCRIBER_OVERRIDE", _boom) + _write_asr_input(tmp_path, [{"audio": "clip.wav"}]) + result = CliRunner().invoke( + cli_app, + ["infer", "--task", "asr", "--model", "m", + "--input", "in.jsonl", "--output", "out.jsonl"], + ) + # All rows skipped → exit 2 (L2), and no raw ESC reaches the terminal. + assert result.exit_code == 2 + assert "\x1b" not in result.output + + +class TestAsrTaskValidation: + def test_bad_asr_task_rejected_upfront(self, tmp_path, monkeypatch): + from soup_cli.cli import app as cli_app + + monkeypatch.chdir(tmp_path) + _write_asr_input(tmp_path, [{"audio": "a.wav"}]) + result = CliRunner().invoke( + cli_app, + ["infer", "--task", "asr", "--model", "m", "--input", "in.jsonl", + "--output", "out.jsonl", "--asr-task", "translat"], + ) + assert result.exit_code == 2 + assert "transcribe" in result.output + + +class TestAsrAllSkippedExit: + def test_all_rows_skipped_exit_2(self, tmp_path, monkeypatch): + import soup_cli.commands.infer as infer + from soup_cli.cli import app as cli_app + + monkeypatch.chdir(tmp_path) + (tmp_path / "a.wav").write_bytes(b"x") + + def _boom(_p): + raise OSError("cannot decode") + + monkeypatch.setattr(infer, "_ASR_TRANSCRIBER_OVERRIDE", _boom) + _write_asr_input(tmp_path, [{"audio": "a.wav"}]) + result = CliRunner().invoke( + cli_app, + ["infer", "--task", "asr", "--model", "m", + "--input", "in.jsonl", "--output", "out.jsonl"], + ) + assert result.exit_code == 2 + assert not (tmp_path / "out.jsonl").exists() + + +# -------------------------------------------------------------------------- +# MEDIUM — distill KD term must shift like the CE term +# -------------------------------------------------------------------------- +class TestDistillKdShift: + def test_mask_aligns_to_predicted_token(self): + import torch + + from soup_cli.trainer.distill import _compute_distill_term + + # seq=4, vocab=3. Teacher/student differ ONLY at position 1 (which, + # after the causal shift, predicts token at index 2). + student = torch.zeros(1, 4, 3) + teacher = torch.zeros(1, 4, 3) + teacher[0, 1, 0] = 10.0 # divergence localised at position 1 + + # Case A: only the PREDICTED token (index 2) is trained → after the + # shift, position 1 is included → non-zero divergence. + labels_a = torch.tensor([[-100, -100, 5, -100]]) + loss_a = _compute_distill_term( + student, teacher, "forward_kl", 1.0, labels=labels_a + ) + assert float(loss_a) > 0.0 + + # Case B: only the INPUT token at index 1 is trained (its prediction, + # index 1's target = index 2, is NOT) → position 1 excluded → ~0. + labels_b = torch.tensor([[-100, 5, -100, -100]]) + loss_b = _compute_distill_term( + student, teacher, "forward_kl", 1.0, labels=labels_b + ) + assert float(loss_b) == pytest.approx(0.0, abs=1e-6) + + +# -------------------------------------------------------------------------- +# MEDIUM — load_config_from_string ValueError-only contract +# -------------------------------------------------------------------------- +class TestLoaderNonMapping: + def test_bare_list_raises_valueerror(self): + from soup_cli.config.loader import load_config_from_string + + with pytest.raises(ValueError, match="mapping"): + load_config_from_string("- a\n- b\n") + + def test_scalar_raises_valueerror(self): + from soup_cli.config.loader import load_config_from_string + + with pytest.raises(ValueError, match="mapping"): + load_config_from_string("42\n") + + +# -------------------------------------------------------------------------- +# MEDIUM — `soup runs` escapes config-derived strings +# -------------------------------------------------------------------------- +class TestRunsEscape: + def _install_fake_tracker(self, monkeypatch, run): + class _FakeTracker: + def __init__(self, *a, **k): + pass + + def list_runs(self, limit=50): + return [run] + + def get_run(self, run_id): + return run + + def get_eval_results(self, run_id=None): + return [] + + def get_metrics(self, run_id): + return [] + + def close(self): + pass + + monkeypatch.setattr( + "soup_cli.experiment.tracker.ExperimentTracker", _FakeTracker + ) + + def _crafted_run(self): + return { + "run_id": "abcd1234", + "experiment_name": "exp [/] boom", + "base_model": "org/[link=http://x]evil[/]", + "task": "sft", + "status": "completed", + "created_at": "2026-07-08T10:00:00", + "total_steps": 10, + "duration_secs": 60, + "device_name": "GPU", + "device": "cuda", + "gpu_memory": "4GB", + "output_dir": "out", + "config_json": None, + } + + def test_list_does_not_crash(self, monkeypatch): + from soup_cli.commands.runs import app as runs_app + + self._install_fake_tracker(monkeypatch, self._crafted_run()) + result = CliRunner().invoke(runs_app, []) + assert result.exit_code == 0, (result.output, repr(result.exception)) + + def test_show_does_not_crash(self, monkeypatch): + from soup_cli.commands.runs import app as runs_app + + self._install_fake_tracker(monkeypatch, self._crafted_run()) + result = CliRunner().invoke(runs_app, ["show", "abcd1234", "--no-plot"]) + assert result.exit_code == 0, (result.output, repr(result.exception)) + + +# -------------------------------------------------------------------------- +# MEDIUM — constant-time UI token compare (behaviour preserved) +# -------------------------------------------------------------------------- +class TestConstantTimeToken: + def test_wrong_token_401_right_token_ok(self): + pytest.importorskip("fastapi") + from starlette.testclient import TestClient + + from soup_cli.ui.app import create_app, set_auth_token + + set_auth_token("a" * 32) + app = create_app() + client = TestClient(app) + # A mutating endpoint requires the Bearer token. + bad = client.post( + "/api/train/start", + headers={"Authorization": "Bearer wrong"}, + json={"config": "base: x"}, + ) + assert bad.status_code == 401 + # Correct token gets past the auth gate (may fail later on validation, + # but never with 401). + ok = client.post( + "/api/train/start", + headers={"Authorization": "Bearer " + "a" * 32}, + json={"config": "not: valid: config"}, + ) + assert ok.status_code != 401 + + +# -------------------------------------------------------------------------- +# MEDIUM — atomic writes for vscode_setup + lr_finder +# -------------------------------------------------------------------------- +class TestAtomicWrites: + def test_vscode_writes_and_honours_force(self, tmp_path, monkeypatch): + from soup_cli.utils.vscode_setup import write_vscode_launch + + monkeypatch.chdir(tmp_path) + out = write_vscode_launch(config_path="soup.yaml", target_dir=".vscode") + assert Path(out).exists() + payload = json.loads(Path(out).read_text()) + assert "configurations" in payload + # Second write without force must refuse. + with pytest.raises(FileExistsError): + write_vscode_launch(config_path="soup.yaml", target_dir=".vscode") + # force=True overwrites atomically. + write_vscode_launch( + config_path="other.yaml", target_dir=".vscode", force=True + ) + assert Path(out).exists() + + def test_lr_finder_writes_under_cwd(self, tmp_path, monkeypatch): + from soup_cli.utils.lr_finder import save_lr_finder_report + + monkeypatch.chdir(tmp_path) + out = tmp_path / "lr.json" + save_lr_finder_report([1e-5, 1e-4, 1e-3, 1e-2], [3.0, 2.0, 2.5, 4.0], out) + payload = json.loads(out.read_text()) + assert "recommended_lr" in payload + + def test_lr_finder_rejects_outside_cwd(self, tmp_path, monkeypatch): + from soup_cli.utils.lr_finder import save_lr_finder_report + + monkeypatch.chdir(tmp_path) + outside = tmp_path.parent / "escape.json" + with pytest.raises(ValueError): + save_lr_finder_report([1e-5, 1e-4], [3.0, 2.0], outside) diff --git a/tests/test_vision.py b/tests/test_vision.py index 2db04cc..fed15e2 100644 --- a/tests/test_vision.py +++ b/tests/test_vision.py @@ -296,14 +296,18 @@ class TestVisionDataLoader: """Test vision image validation in data loader.""" def test_validate_vision_images_resolves_relative(self): - """Relative image paths should be resolved against image_dir.""" + """Relative image paths are resolved against image_dir (realpath). + + v0.71.33: the validator now stores the resolved path (mirrors + _validate_audio_files) after a containment check. + """ from soup_cli.data.loader import _validate_vision_images data = [{"messages": [{"role": "user", "content": "Hi"}], "image": "photo.jpg"}] image_dir = Path("/data/images") result = _validate_vision_images(data, image_dir) assert len(result) == 1 - assert result[0]["image"] == str(image_dir / "photo.jpg") + assert result[0]["image"] == str((image_dir / "photo.jpg").resolve()) def test_validate_vision_images_skips_missing(self): """Rows without image field should be skipped.""" @@ -326,15 +330,29 @@ class TestVisionDataLoader: result = _validate_vision_images(data, image_dir) assert len(result) == 0 - def test_validate_vision_images_absolute_path(self): - """Absolute image paths should be kept as-is.""" + def test_validate_vision_images_absolute_inside_kept(self, tmp_path): + """An absolute image path INSIDE image_dir is kept (resolved).""" from soup_cli.data.loader import _validate_vision_images - abs_path = str(Path("/absolute/path/photo.jpg")) - data = [{"messages": [{"role": "user", "content": "Hi"}], "image": abs_path}] - image_dir = Path("/different/dir") - result = _validate_vision_images(data, image_dir) - assert result[0]["image"] == abs_path + img_dir = tmp_path / "imgs" + img_dir.mkdir() + inside = img_dir / "photo.jpg" + inside.write_bytes(b"x") + data = [{"messages": [{"role": "user", "content": "Hi"}], "image": str(inside)}] + result = _validate_vision_images(data, img_dir) + assert len(result) == 1 + assert Path(result[0]["image"]).name == "photo.jpg" + + def test_validate_vision_images_rejects_out_of_dir(self, tmp_path): + """v0.71.33 security fix: an absolute path OUTSIDE image_dir is dropped + (previously it was handed straight to PIL.Image.open — arbitrary read).""" + from soup_cli.data.loader import _validate_vision_images + + img_dir = tmp_path / "imgs" + img_dir.mkdir() + data = [{"messages": [{"role": "user", "content": "Hi"}], "image": "/etc/passwd"}] + result = _validate_vision_images(data, img_dir) + assert result == [] def test_load_dataset_with_vision_format(self): """load_dataset should handle llava format data files.""" diff --git a/tests/test_vllm_serve.py b/tests/test_vllm_serve.py index f8401a9..d8ad24c 100644 --- a/tests/test_vllm_serve.py +++ b/tests/test_vllm_serve.py @@ -505,6 +505,7 @@ class TestServeVllmIntegration: num_speculative_tokens=5, enable_prefix_caching=False, quantization=None, # v0.35.0 #61 — auto-quant default + trust_remote_code=False, # v0.71.33 — default-deny gate ) mock_create_app.assert_called_once()