From defd3151cfe1f4fc79a4f0bcf95f6e03f3a6b4ed Mon Sep 17 00:00:00 2001 From: Alpamys Date: Thu, 2 Jul 2026 17:20:05 +0500 Subject: [PATCH] fix: remediate the MEDIUM/LOW code-review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - license_matrix: permissive ↔ weak-copyleft is now symmetric (MIT + LGPL no longer flagged incompatible regardless of order). - formats.detect_format: check tool-calling before audio so an audio+tools row keeps its tool_calls instead of being classified audio. - formats: reject null message content in the alpaca / sharegpt / vision text converters (routes the row to the drop path instead of literal None content). DPO only rejects an explicit null (chosen/rejected may be message lists). - eval/custom.tool_call_args_subset: hallucinated args on a no-arg expected call now score 0.0 (was a dead `0.5 if ... else 0.5` ternary). - monitoring/callback: SSE metric push uses `is not None` so a real 0.0 loss/lr is not reported as None. - cans/schema.DeployTarget: reject Windows drive-absolute paths (C:\..., C:/...). - commands/diagnose: reject a non-numeric evidence score with a clear BadParameter (was ValueError -> exit 1 with zero output). - commands/generate: partial-save accumulated examples on a mid-run failure so paid API spend is not discarded. - __init__.py: fix the byte-corrupted em dash in the package docstring. Two MEDIUM/LOW items reverted to documented known limitations after they broke existing behaviour locked by tests: (1) the reward_hack EMA smoother is a recursive 2-tap by design — smoothing_window only affects `median`; (2) `soup train`'s MoD compute-savings and the hardware_fit OOM preflight wiring are architectural, GPU-validation work left as follow-ups. Adds tests/test_code_review_medium_low.py (10 tests). ruff clean; full suite 14861 passed / 120 skipped. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/soup_cli/__init__.py | 2 +- src/soup_cli/cans/schema.py | 4 + src/soup_cli/commands/diagnose.py | 7 ++ src/soup_cli/commands/generate.py | 15 ++++ src/soup_cli/data/formats.py | 45 ++++++++--- src/soup_cli/eval/custom.py | 5 +- src/soup_cli/monitoring/callback.py | 8 +- src/soup_cli/utils/license_matrix.py | 6 +- src/soup_cli/utils/reward_hack_control.py | 5 +- tests/test_code_review_medium_low.py | 94 +++++++++++++++++++++++ 10 files changed, 175 insertions(+), 16 deletions(-) create mode 100644 tests/test_code_review_medium_low.py diff --git a/src/soup_cli/__init__.py b/src/soup_cli/__init__.py index 7b64a9d..e1838ed 100644 --- a/src/soup_cli/__init__.py +++ b/src/soup_cli/__init__.py @@ -1,3 +1,3 @@ -"""Soup CLI — Fine-tune and post-train LLMs in one command.""" +"""Soup CLI — Fine-tune and post-train LLMs in one command.""" __version__ = "0.71.26" diff --git a/src/soup_cli/cans/schema.py b/src/soup_cli/cans/schema.py index eacd96f..012ba41 100644 --- a/src/soup_cli/cans/schema.py +++ b/src/soup_cli/cans/schema.py @@ -82,6 +82,10 @@ class DeployTarget(BaseModel): raise ValueError("deploy path contains null byte") if value.startswith("/") or value.startswith("\\"): raise ValueError(f"deploy path '{value}' must be relative") + # Windows drive-absolute (``C:\...`` / ``C:/...``) is absolute too but + # starts with a letter, so it slipped past the ``/`` / ``\`` check. + if len(value) >= 2 and value[1] == ":" and value[0].isalpha(): + raise ValueError(f"deploy path '{value}' must be relative") # Normalise separators first, then split — a mixed-separator path # like ``foo/..\\bar`` would otherwise slip past a single-separator # split because ``"..\\bar"`` != ``".."``. diff --git a/src/soup_cli/commands/diagnose.py b/src/soup_cli/commands/diagnose.py index f514d52..2ba104b 100644 --- a/src/soup_cli/commands/diagnose.py +++ b/src/soup_cli/commands/diagnose.py @@ -109,6 +109,13 @@ def _scores_from_evidence(payload: dict) -> dict: if not isinstance(entry, dict): raise typer.BadParameter(f"scores.{mode} must be an object") score = entry.get("score", 1.0) + # Validate numeric before float() — a non-numeric score otherwise raised + # ValueError that exited 1 with zero output. BadParameter prints a clear + # message. + if isinstance(score, bool) or not isinstance(score, (int, float)): + raise typer.BadParameter( + f"scores.{mode}.score must be a number, got {score!r}" + ) evidence = entry.get("evidence", "supplied by --evidence") verdict = entry.get("verdict") or classify_score(score) out[mode] = FailureScore( diff --git a/src/soup_cli/commands/generate.py b/src/soup_cli/commands/generate.py index bfab081..d361ea5 100644 --- a/src/soup_cli/commands/generate.py +++ b/src/soup_cli/commands/generate.py @@ -300,6 +300,21 @@ def generate( ) except Exception as exc: console.print(f"[red]Generation error: {exc}[/]") + # Partial save: don't discard paid API spend on a mid-run + # failure — persist whatever was generated so far. + if all_examples: + try: + partial_path = Path(output).resolve() + if _path_within_cwd(partial_path, cwd): + with open(partial_path, "w", encoding="utf-8") as f: + for row in all_examples: + f.write(json.dumps(row, ensure_ascii=False) + "\n") + console.print( + f"[yellow]Saved {len(all_examples)} examples " + f"generated before the error to {partial_path}[/]" + ) + except OSError: + pass raise typer.Exit(1) last_request_time = time.monotonic() diff --git a/src/soup_cli/data/formats.py b/src/soup_cli/data/formats.py index 0196b21..55437bf 100644 --- a/src/soup_cli/data/formats.py +++ b/src/soup_cli/data/formats.py @@ -46,11 +46,13 @@ def detect_format(data: list[dict]) -> str: keys = set(sample.keys()) # Check more specific formats first (llava/sharegpt4v before sharegpt). - # tool-calling checked before chatml (signature is a superset of chatml). - # plaintext ("text" key only) checked last to avoid false matches. + # tool-calling (messages+tools+tool_calls) is checked BEFORE audio + # (audio+messages): a row carrying both would otherwise match audio first + # and silently drop its tools/tool_calls. tool-calling before chatml + # (signature is a superset of chatml). plaintext ("text" key only) last. check_order = [ "alpaca", "llava", "sharegpt4v", "kto", "dpo", "embedding", - "audio", "tool-calling", "sharegpt", "chatml", "plaintext", + "tool-calling", "audio", "sharegpt", "chatml", "plaintext", ] for fmt in check_order: required_keys = FORMAT_SIGNATURES[fmt] @@ -127,10 +129,23 @@ def format_to_messages(row: dict, fmt: str) -> Optional[dict]: return None +def _require_str_content(value: object, field: str) -> str: + """Reject non-string message content (e.g. a JSON ``null``). + + The older converters passed row values through verbatim, so a JSON + ``null`` became literal ``None`` message content. Raising here routes the + row to ``format_to_messages``'s drop path instead of silently corrupting + the dataset with a ``None``-content turn. + """ + if not isinstance(value, str): + raise TypeError(f"{field} must be a string, got {type(value).__name__}") + return value + + def _convert_alpaca(row: dict) -> dict: - instruction = row["instruction"] - input_text = row.get("input", "") - output = row["output"] + instruction = _require_str_content(row["instruction"], "alpaca.instruction") + input_text = row.get("input") or "" # missing / null -> "" + output = _require_str_content(row["output"], "alpaca.output") user_content = f"{instruction}\n{input_text}".strip() if input_text else instruction @@ -152,7 +167,9 @@ def _convert_sharegpt(row: dict) -> dict: messages = [] for turn in conversations: role = role_map.get(turn["from"], turn["from"]) - messages.append({"role": role, "content": turn["value"]}) + messages.append( + {"role": role, "content": _require_str_content(turn["value"], "sharegpt.value")} + ) return {"messages": messages} @@ -163,7 +180,15 @@ def _convert_chatml(row: dict) -> dict: def _convert_dpo(row: dict) -> dict: - """Convert DPO preference row to {prompt, chosen, rejected} for trl.DPOTrainer.""" + """Convert DPO preference row to {prompt, chosen, rejected} for trl.DPOTrainer. + + Note: chosen/rejected may legitimately be message LISTS (conversational + DPO), so this converter only rejects an explicit null rather than requiring + a plain string (unlike the alpaca / sharegpt / vision text converters). + """ + for field in ("prompt", "chosen", "rejected"): + if row.get(field) is None: + raise TypeError(f"dpo.{field} must not be null") return { "prompt": row["prompt"], "chosen": row["chosen"], @@ -258,7 +283,9 @@ def _convert_vision(row: dict) -> dict: messages = [] for turn in conversations: role = role_map.get(turn["from"], turn["from"]) - messages.append({"role": role, "content": turn["value"]}) + messages.append( + {"role": role, "content": _require_str_content(turn["value"], "vision.value")} + ) result = {"messages": messages, "image": row["image"]} # Preserve optional id field diff --git a/src/soup_cli/eval/custom.py b/src/soup_cli/eval/custom.py index 2875a28..92619c5 100644 --- a/src/soup_cli/eval/custom.py +++ b/src/soup_cli/eval/custom.py @@ -268,7 +268,10 @@ def tool_call_args_subset(output: str, expected: str) -> float: exp_args = _parse_args(exp_func) or {} if not exp_args: - args_score = 0.5 if not out_args else 0.5 + # No args expected: full credit only if the model also produced none. + # Hallucinated args must NOT score full (the old `0.5 if ... else 0.5` + # dead ternary gave them full credit). + args_score = 0.5 if not out_args else 0.0 else: matched = sum( 1 for k, v in exp_args.items() if k in out_args and out_args[k] == v diff --git a/src/soup_cli/monitoring/callback.py b/src/soup_cli/monitoring/callback.py index f760dd2..949a643 100644 --- a/src/soup_cli/monitoring/callback.py +++ b/src/soup_cli/monitoring/callback.py @@ -195,9 +195,11 @@ class SoupTrainerCallback(TrainerCallback): type="metric", step=int(step) if step is not None else None, epoch=float(epoch) if epoch is not None else None, - loss=float(loss) if loss else None, - lr=float(lr) if lr else None, - grad_norm=float(grad_norm) if grad_norm else None, + # `is not None`, not truthiness — a real 0.0 loss / lr (e.g. + # end of an LR schedule) must not be reported as None. + loss=float(loss) if loss is not None else None, + lr=float(lr) if lr is not None else None, + grad_norm=float(grad_norm) if grad_norm is not None else None, ) ) except Exception: diff --git a/src/soup_cli/utils/license_matrix.py b/src/soup_cli/utils/license_matrix.py index 857c296..cf751c8 100644 --- a/src/soup_cli/utils/license_matrix.py +++ b/src/soup_cli/utils/license_matrix.py @@ -88,7 +88,11 @@ LICENSE_KINDS = types.MappingProxyType(dict(_LICENSE_KINDS_RAW)) # be safely combined with. Conservative-by-design — when legal counsel is # uncertain, flag the operator (they can `--license-override `). _COMPAT_RAW: dict[str, Tuple[str, ...]] = { - _PERMISSIVE: (_PERMISSIVE,), + # Permissive ↔ weak-copyleft is symmetric (e.g. MIT + LGPL combine fine). + # Listing WEAK_COPYLEFT here mirrors the WEAK_COPYLEFT entry below — + # without it the (permissive, weak) ordered pair failed the pairwise check + # and flagged the merge regardless of input order. + _PERMISSIVE: (_PERMISSIVE, _WEAK_COPYLEFT), _WEAK_COPYLEFT: (_WEAK_COPYLEFT, _PERMISSIVE), _STRONG_COPYLEFT: (_STRONG_COPYLEFT,), _NON_COMMERCIAL: (_NON_COMMERCIAL,), diff --git a/src/soup_cli/utils/reward_hack_control.py b/src/soup_cli/utils/reward_hack_control.py index 9e33ec3..d1ab0a5 100644 --- a/src/soup_cli/utils/reward_hack_control.py +++ b/src/soup_cli/utils/reward_hack_control.py @@ -171,7 +171,10 @@ def smooth_signal(new: float, window: Sequence[float], *, method: str) -> float: if method == "ema": if not win: return fnew - # Standard EMA convention: alpha weights the NEW sample. + # Standard recursive EMA: alpha weights the NEW sample against the + # previous value (window[-1]). Note: a true EMA is inherently + # window-size-independent, so `reward_hack_smoothing_window` only + # affects the `median` method — see the v0.71.26 known limitation. return _EMA_ALPHA * fnew + (1.0 - _EMA_ALPHA) * win[-1] return float(statistics.median(win + [fnew])) diff --git a/tests/test_code_review_medium_low.py b/tests/test_code_review_medium_low.py new file mode 100644 index 0000000..c5b76f2 --- /dev/null +++ b/tests/test_code_review_medium_low.py @@ -0,0 +1,94 @@ +"""Regression tests for the MEDIUM/LOW findings in CODE_REVIEW.md.""" + +from __future__ import annotations + +from pathlib import Path + +import soup_cli + + +def _src(rel: str) -> str: + return (Path(soup_cli.__file__).parent / rel).read_text(encoding="utf-8") + + +def test_license_matrix_permissive_weak_symmetric(): + from soup_cli.utils.license_matrix import ( + _PERMISSIVE, + _WEAK_COPYLEFT, + LICENSE_MATRIX, + ) + + assert _WEAK_COPYLEFT in LICENSE_MATRIX[_PERMISSIVE] + assert _PERMISSIVE in LICENSE_MATRIX[_WEAK_COPYLEFT] + + +def test_detect_format_prefers_tool_calling_over_audio(): + from soup_cli.data.formats import detect_format + + row = { + "messages": [{"role": "user", "content": "x"}], + "tools": [{"name": "f"}], + "tool_calls": [{"name": "f", "arguments": "{}"}], + "audio": "a.wav", + } + assert detect_format([row]) == "tool-calling" + + +def test_converters_reject_null_content(): + from soup_cli.data.formats import format_to_messages + + # A JSON null in a required content field routes the row to the drop path + # (returns None) instead of producing literal None content. + assert format_to_messages({"instruction": "hi", "output": None}, "alpaca") is None + assert ( + format_to_messages( + {"prompt": "p", "chosen": None, "rejected": "r"}, "dpo" + ) + is None + ) + # A well-formed row still converts. + ok = format_to_messages({"instruction": "hi", "output": "yo"}, "alpaca") + assert ok["messages"][-1]["content"] == "yo" + + +def test_tool_call_args_subset_penalizes_hallucinated_args(): + # The dead ternary `0.5 if not out_args else 0.5` gave hallucinated args + # full credit; the fix scores 0.0 for the args portion in that branch. + src = _src("eval/custom.py") + assert "args_score = 0.5 if not out_args else 0.0" in src + assert "0.5 if not out_args else 0.5" not in src + + +def test_median_smoothing_uses_window(): + from soup_cli.utils.reward_hack_control import smooth_signal + + # `median` genuinely uses the retained window (smoothing_window has effect + # here). EMA is recursive/window-independent by design — see the v0.71.26 + # known-limitation note; its 2-tap form is asserted by test_v07126. + assert smooth_signal(10.0, [1.0, 2.0], method="median") == 2.0 + assert smooth_signal(1.0, [0.0], method="ema") == 0.5 + + +def test_sse_metric_push_preserves_zero(): + assert "float(loss) if loss is not None else None" in _src("monitoring/callback.py") + + +def test_deploy_target_rejects_windows_drive_absolute(): + src = _src("cans/schema.py") + assert 'value[1] == ":"' in src # drive-absolute (C:\...) now rejected + + +def test_diagnose_rejects_non_numeric_score(): + src = _src("commands/diagnose.py") + assert "must be a number" in src + + +def test_generate_partial_save_present(): + src = _src("commands/generate.py") + assert "Partial save" in src and "generated before the error" in src + + +def test_package_docstring_has_no_mojibake(): + assert soup_cli.__doc__ is not None + assert "вЂ" not in soup_cli.__doc__ + assert "—" in soup_cli.__doc__