fix(v0.71.36): address ECC python-review findings

No CRITICAL/HIGH. All MEDIUM/LOW fixed per the project's fix-every-finding
rule. The review cited this codebase's OWN precedent rather than generic
opinion, so every fix matches an existing sibling.

Type precision (the bulk):
- parameterize frozen-dataclass generics: DedupReport.kept/dropped/pairs,
  Topic.terms/member_indices, TopicReport.topics/warnings,
  CanaryReport.exposures -- matches best_of_n.py's existing pattern
- real signatures on canary.py's public API (Sequence[Canary],
  tuple[Canary, ...], list[dict], dict[str, Any], Collection[str]),
  topics.ctfidf_labels/build_topic_report, rehearsal.mix_replay
- numpy-carrying params/returns follow spectrum_scan.py: a TYPE_CHECKING
  `from numpy.typing import NDArray` + quoted "NDArray[Any]", keeping numpy
  a lazy runtime import
- data.py::_semantic_dedup was the only untyped helper in a fully-typed
  file -> list[dict] + -> DedupReport
- loader.py::_load_replay_rows -> list[dict], matching every sibling _load_*
- data_canary.py::_load_pair typed via TYPE_CHECKING, mirroring
  commands/draft.py::_load_pair_member

Idiom consistency:
- math.isnan() replaces the `x == x` trick in canary.py, live_eval.py and
  data_canary.py; isnan is the dominant idiom across ~6 existing modules,
  and canary.py already imported math. Verified the swap preserves
  behaviour: a non-numeric control still raises TypeError rather than
  silently scoring.
- schema.py: getattr(self.training, "multipack", False) -> direct access.
  multipack is a required field, so the defensive getattr implied it might
  be missing.
- data_topics.py::_parse_clusters now chains with `from exc`
- loader.py: is_under_cwd moved to the top-level imports -- utils.paths has
  no heavy deps, so the lazy import served no purpose

Diagnosability:
- embed.py::_fetch_pooling_config logs the real exception at debug level
  before returning None. The breadth stays (it is fail-CLOSED by design --
  the caller refuses), but a hub outage and an absent file were
  indistinguishable to a user debugging a transient failure.
- documented WHY dedup and topics pick row text differently, since a reader
  meeting both cold would wonder

254 tests green, ruff clean.
This commit is contained in:
Alpamys 2026-07-16 18:05:58 +05:00
parent 64e80dd142
commit 5dd9917cee
11 changed files with 123 additions and 52 deletions

View File

@ -16,7 +16,7 @@ from soup_cli.data.loader import load_raw_data
from soup_cli.data.validator import validate_and_stats
from soup_cli.utils.embed import DEFAULT_EMBED_MODEL, embed_texts
from soup_cli.utils.paths import is_under_cwd
from soup_cli.utils.semdedup import greedy_semdedup
from soup_cli.utils.semdedup import DedupReport, greedy_semdedup
console = Console()
@ -237,7 +237,10 @@ def _row_embed_text(row: dict, field: Optional[str]) -> str:
"""What gets embedded for a row: one field, or all text values joined.
Mirrors the MinHash branch's text selection so ``--field`` means the
same thing for both backends.
same thing for both backends. NOTE: ``soup data topics`` deliberately
picks row text differently it prefers ``_eval_text.row_text``'s
assistant-turn extraction, because a topic map should cluster on what
the model is taught to SAY, whereas dedup must consider the whole row.
"""
if field:
return str(row.get(field, ""))
@ -245,14 +248,14 @@ def _row_embed_text(row: dict, field: Optional[str]) -> str:
def _semantic_dedup(
data: list,
data: list[dict],
*,
threshold: float,
field: Optional[str],
embed_model: str,
device: str,
out_path: Path,
):
) -> DedupReport:
"""SemDeDup branch of ``soup data dedup --semantic``."""
texts = [_row_embed_text(row, field) for row in data]
try:

View File

@ -11,8 +11,9 @@ Mirrors ``soup diagnose`` / ``soup ship`` exit conventions so CI can gate.
from __future__ import annotations
import json
import math
from pathlib import Path
from typing import Optional
from typing import TYPE_CHECKING, Optional
import typer
from rich.console import Console
@ -32,6 +33,9 @@ from soup_cli.utils.canary import (
from soup_cli.utils.live_eval import compute_pair_losses
from soup_cli.utils.paths import atomic_write_text, is_under_cwd
if TYPE_CHECKING: # static types only — transformers stays a lazy import
from transformers import PreTrainedModel, PreTrainedTokenizerBase
console = Console()
app = typer.Typer(
no_args_is_help=True, help="Dataset canaries (memorization probe)."
@ -43,7 +47,9 @@ _CONTROL_SEED = 12345
_VERDICT_COLOUR = {"OK": "green", "MINOR": "yellow", "MAJOR": "red"}
def _load_pair(base: str, adapter: Optional[str], device: str):
def _load_pair(
base: str, adapter: Optional[str], device: str
) -> "tuple[PreTrainedModel, PreTrainedTokenizerBase, str]":
"""Seam: load ``(model, tokenizer, device)``. Patched in tests."""
from soup_cli.utils.live_eval import load_model_and_tokenizer
@ -184,7 +190,7 @@ def check(
table.add_column("Memorized", justify="right")
for exposure in report.exposures:
loss_text = (
"nan" if exposure.loss != exposure.loss else f"{exposure.loss:.4f}"
"nan" if math.isnan(exposure.loss) else f"{exposure.loss:.4f}"
)
table.add_row(
escape(exposure.secret.strip()),

View File

@ -50,16 +50,16 @@ def _report_to_dict(report) -> dict:
}
def _parse_clusters(value: str):
def _parse_clusters(value: str) -> "int | str":
"""``"auto"`` stays a string; anything else must be an int."""
if value.strip().lower() == "auto":
return "auto"
try:
return int(value)
except ValueError:
except ValueError as exc:
raise ValueError(
f"--clusters must be an integer or 'auto', got {value!r}"
)
) from exc
def topics(

View File

@ -4684,7 +4684,7 @@ class SoupConfig(BaseModel):
"(packing concatenates rows into fixed blocks, so the "
"replay ratio stops being meaningful)"
)
if getattr(self.training, "multipack", False):
if self.training.multipack:
raise ValueError(
"data.replay is incompatible with training.multipack "
"(bin-packing breaks the replay ratio)"

View File

@ -14,6 +14,7 @@ from soup_cli.data.formats import (
is_audio_format,
is_vision_format,
)
from soup_cli.utils.paths import is_under_cwd
console = Console()
@ -110,14 +111,12 @@ def _load_txt(path: Path) -> list[dict]:
return [{"text": line} for line in lines]
def _load_replay_rows(data_config: DataConfig) -> list:
def _load_replay_rows(data_config: DataConfig) -> list[dict]:
"""Load + normalize the replay file with its OWN format detection.
The old dataset may be alpaca while the new one is sharegpt, so the
replay file cannot inherit ``data_config.format``.
"""
from soup_cli.utils.paths import is_under_cwd
replay_path = Path(data_config.replay)
if not is_under_cwd(replay_path):
raise ValueError(
@ -134,7 +133,10 @@ def _load_replay_rows(data_config: DataConfig) -> list:
def _finalize(
formatted: list, data_config: DataConfig, *, val: list | None = None
formatted: list[dict],
data_config: DataConfig,
*,
val: list[dict] | None = None,
) -> dict:
"""Split train/val, then mix replay into train ONLY.

View File

@ -22,7 +22,7 @@ import math
import os
import random
from dataclasses import dataclass
from typing import Collection
from typing import Any, Collection, Sequence
from soup_cli.utils.paths import atomic_write_text, enforce_under_cwd_and_no_symlink
@ -60,7 +60,7 @@ class CanaryExposure:
@dataclass(frozen=True)
class CanaryReport:
exposures: tuple
exposures: tuple[CanaryExposure, ...]
n_controls: int
verdict: str
@ -83,7 +83,9 @@ def _make_secret(rng: random.Random) -> str:
return " " + "-".join(groups)
def _generate(count: int, seed: int, exclude: Collection) -> tuple:
def _generate(
count: int, seed: int, exclude: Collection[str]
) -> tuple[Canary, ...]:
rng = random.Random(seed)
carrier = CARRIER_TEMPLATE.format(slug=_SLUG)
seen = set(exclude)
@ -97,12 +99,14 @@ def _generate(count: int, seed: int, exclude: Collection) -> tuple:
return tuple(out)
def generate_canaries(*, count: int, seed: int) -> tuple:
def generate_canaries(*, count: int, seed: int) -> tuple[Canary, ...]:
"""K unique canaries, deterministic in ``seed``."""
return _generate(_require_count(count, "count"), seed, ())
def generate_controls(*, count: int, seed: int, exclude: Collection) -> tuple:
def generate_controls(
*, count: int, seed: int, exclude: Collection[str]
) -> tuple[Canary, ...]:
"""N controls from the SAME space, sharing the carrier, never inserted.
Sharing the carrier is what isolates the secret: if the controls used a
@ -111,7 +115,7 @@ def generate_controls(*, count: int, seed: int, exclude: Collection) -> tuple:
return _generate(_require_count(count, "count"), seed, exclude)
def canary_rows(canaries) -> list:
def canary_rows(canaries: Sequence[Canary]) -> list[dict]:
"""Render canaries as ``{"messages": [...]}`` training rows."""
return [
{
@ -124,7 +128,7 @@ def canary_rows(canaries) -> list:
]
def write_manifest(canaries, path: str) -> str:
def write_manifest(canaries: Sequence[Canary], path: str) -> str:
"""Persist the secrets. THIS FILE IS THE SENSITIVE ARTIFACT.
Anyone holding it can reproduce the canaries, so it must not be
@ -144,7 +148,7 @@ def write_manifest(canaries, path: str) -> str:
return safe
def load_manifest(path: str) -> tuple:
def load_manifest(path: str) -> tuple[Canary, ...]:
"""Read a canary manifest written by :func:`write_manifest`."""
safe = enforce_under_cwd_and_no_symlink(str(path), "manifest")
if os.path.getsize(safe) > _MAX_MANIFEST_BYTES:
@ -175,7 +179,11 @@ def load_manifest(path: str) -> tuple:
return tuple(out)
def compute_exposure(canary_losses, control_losses, secrets) -> tuple:
def compute_exposure(
canary_losses: Sequence[float],
control_losses: Sequence[float],
secrets: Sequence[str],
) -> tuple[CanaryExposure, ...]:
"""Rank each canary's loss against the control distribution.
``percentile`` = fraction of controls STRICTLY cheaper than the canary.
@ -196,7 +204,7 @@ def compute_exposure(canary_losses, control_losses, secrets) -> tuple:
strongest possible leak.
"""
losses = list(canary_losses)
controls = [value for value in control_losses if value == value] # drop nan
controls = [value for value in control_losses if not math.isnan(value)]
secret_list = list(secrets)
if len(losses) != len(secret_list):
raise ValueError(
@ -235,7 +243,7 @@ def compute_exposure(canary_losses, control_losses, secrets) -> tuple:
return tuple(out)
def classify_canary(exposures) -> str:
def classify_canary(exposures: Sequence[CanaryExposure]) -> str:
"""OK / MINOR / MAJOR — the single source of truth for the verdict."""
items = list(exposures)
if not items:
@ -249,17 +257,21 @@ def classify_canary(exposures) -> str:
return "OK"
def build_canary_report(canary_losses, control_losses, secrets) -> CanaryReport:
def build_canary_report(
canary_losses: Sequence[float],
control_losses: Sequence[float],
secrets: Sequence[str],
) -> CanaryReport:
"""Exposure + verdict in one frozen object (mirrors ship_verdict.py)."""
exposures = compute_exposure(canary_losses, control_losses, secrets)
return CanaryReport(
exposures=exposures,
n_controls=len([v for v in control_losses if v == v]),
n_controls=len([v for v in control_losses if not math.isnan(v)]),
verdict=classify_canary(exposures),
)
def canary_report_to_dict(report: CanaryReport) -> dict:
def canary_report_to_dict(report: CanaryReport) -> dict[str, Any]:
"""JSON-safe rendering. NaN losses become null, never 0.0."""
return {
"verdict": report.verdict,
@ -267,7 +279,9 @@ def canary_report_to_dict(report: CanaryReport) -> dict:
"exposures": [
{
"secret": exposure.secret,
"loss": None if exposure.loss != exposure.loss else exposure.loss,
"loss": (
None if math.isnan(exposure.loss) else exposure.loss
),
"percentile": exposure.percentile,
"memorized": exposure.memorized,
}

View File

@ -12,8 +12,11 @@ importable on the light core.
from __future__ import annotations
import json
import logging
from typing import Optional
logger = logging.getLogger(__name__)
# Models whose pooling is verified pure-mean. Short-circuits the hub fetch.
# all-mpnet-base-v2 already ships as the `ra-dit-retriever` recipe base.
POOLING_ALLOWLIST: dict[str, str] = {
@ -57,7 +60,10 @@ def _fetch_pooling_config(model_id: str) -> Optional[dict]:
"""Read ``1_Pooling/config.json`` from the hub repo.
Returns None when the file is absent or unreadable the caller then
REFUSES rather than assuming mean pooling.
REFUSES rather than assuming mean pooling, so every failure here is
fail-CLOSED. The breadth of the ``except`` is therefore safe, but a
hub outage and a genuinely-absent file are indistinguishable to the
user, so the reason is logged at debug level before returning.
"""
try:
from huggingface_hub import hf_hub_download
@ -68,7 +74,13 @@ def _fetch_pooling_config(model_id: str) -> Optional[dict]:
with open(path, "r", encoding="utf-8") as handle:
data = json.load(handle)
return data if isinstance(data, dict) else None
except Exception:
except Exception as exc: # noqa: BLE001 — fail-closed; caller refuses
logger.debug(
"could not fetch 1_Pooling/config.json for %r: %s: %s",
model_id,
type(exc).__name__,
exc,
)
return None

View File

@ -24,6 +24,7 @@ model load is covered by the release-step-6 smoke on SmolLM2-135M.
from __future__ import annotations
import math
import re
import time
from collections.abc import Mapping, Sequence
@ -349,7 +350,7 @@ def compute_eval_loss(
for value in compute_pair_losses(
model, tokenizer, pairs, device=device, max_length=max_length
)
if value == value # drop nan
if not math.isnan(value)
]
if not losses:
return float("nan")

View File

@ -20,6 +20,7 @@ from __future__ import annotations
import math
import random
from dataclasses import dataclass
from typing import Optional, Sequence
@dataclass(frozen=True)
@ -67,7 +68,13 @@ def resolve_replay_count(n_new: int, ratio: float) -> int:
return int(round(rate / (1.0 - rate) * rows))
def mix_replay(new_rows, replay_rows, *, ratio: float, seed=None):
def mix_replay(
new_rows: Sequence[dict],
replay_rows: Sequence[dict],
*,
ratio: float,
seed: Optional[int] = None,
) -> tuple[list[dict], ReplayReport]:
"""Interleave a seeded sample of ``replay_rows`` into ``new_rows``.
Interleaved, NOT appended. ``new + replay`` would put every replay row

View File

@ -20,6 +20,10 @@ from __future__ import annotations
import math
from dataclasses import dataclass
from typing import TYPE_CHECKING, Any
if TYPE_CHECKING: # static types only — numpy stays a lazy runtime import
from numpy.typing import NDArray
# O(n^2) in the worst case — refuse loudly rather than silently subsample.
_MAX_SEMDEDUP_ROWS = 50_000
@ -34,9 +38,9 @@ class DedupReport:
makes a MinHash-vs-semantic comparison checkable rather than a vibe.
"""
kept: tuple
dropped: tuple
pairs: tuple
kept: tuple[int, ...]
dropped: tuple[int, ...]
pairs: tuple[tuple[int, int, float], ...]
threshold: float
@ -53,7 +57,9 @@ def _require_threshold(value: object) -> float:
return num
def greedy_semdedup(vectors, *, threshold: float) -> DedupReport:
def greedy_semdedup(
vectors: "Any | NDArray[Any]", *, threshold: float
) -> DedupReport:
"""Greedy cosine near-duplicate removal over L2-normalized ``vectors``.
Rows are visited in order. The first row of any near-duplicate cluster
@ -76,9 +82,9 @@ def greedy_semdedup(vectors, *, threshold: float) -> DedupReport:
"Soup refuses rather than silently subsampling."
)
kept: list = []
dropped: list = []
pairs: list = []
kept: list[int] = []
dropped: list[int] = []
pairs: list[tuple[int, int, float]] = []
for idx in range(n_rows):
if not kept:
kept.append(idx)

View File

@ -17,6 +17,10 @@ from __future__ import annotations
import math
from dataclasses import dataclass
from typing import TYPE_CHECKING, Any, Sequence
if TYPE_CHECKING: # static types only — numpy stays a lazy runtime import
from numpy.typing import NDArray
_MAX_TOPIC_ROWS = 200_000
# 'auto' never proposes more than this, so the coverage table stays one
@ -29,21 +33,21 @@ class Topic:
"""One cluster: its label, its share of the corpus, and its members."""
label: str
terms: tuple
terms: tuple[str, ...]
size: int
fraction: float
member_indices: tuple
member_indices: tuple[int, ...]
@dataclass(frozen=True)
class TopicReport:
topics: tuple
topics: tuple[Topic, ...]
n_rows: int
n_clusters: int
warnings: tuple
warnings: tuple[str, ...]
def resolve_k(n_rows: int, requested) -> int:
def resolve_k(n_rows: int, requested: "int | str") -> int:
"""Pick the cluster count.
An explicit int is honoured (clamped to ``[1, n_rows]``). ``"auto"``
@ -70,7 +74,13 @@ def resolve_k(n_rows: int, requested) -> int:
return max(1, min(requested, n_rows)) if n_rows else 1
def kmeans(vectors, *, k: int, seed: int, max_iter: int = 50):
def kmeans(
vectors: "Any | NDArray[Any]",
*,
k: int,
seed: int,
max_iter: int = 50,
) -> "NDArray[Any]":
"""Deterministic k-means++ over ``vectors``; returns integer labels."""
import numpy as np
@ -117,7 +127,13 @@ def kmeans(vectors, *, k: int, seed: int, max_iter: int = 50):
return labels
def ctfidf_labels(token_docs, labels, *, k: int, top_n: int = 3) -> list:
def ctfidf_labels(
token_docs: Sequence[Sequence[str]],
labels: Sequence[int],
*,
k: int,
top_n: int = 3,
) -> list[tuple[str, ...]]:
"""Class-based TF-IDF: each CLUSTER is one document.
A term frequent in one cluster and rare across clusters scores high; a
@ -137,7 +153,7 @@ def ctfidf_labels(token_docs, labels, *, k: int, top_n: int = 3) -> list:
if isinstance(top_n, bool) or not isinstance(top_n, int) or top_n < 1:
raise ValueError("top_n must be an int >= 1")
per_class: list = [Counter() for _ in range(k)]
per_class: list[Counter] = [Counter() for _ in range(k)]
for tokens, label in zip(docs, label_list):
idx = int(label)
if 0 <= idx < k:
@ -167,7 +183,11 @@ def ctfidf_labels(token_docs, labels, *, k: int, top_n: int = 3) -> list:
def build_topic_report(
rows, labels, *, k: int, min_fraction: float = 0.02
rows: Sequence[dict],
labels: Sequence[int],
*,
k: int,
min_fraction: float = 0.02,
) -> TopicReport:
"""Assemble the coverage table + gap warnings from cluster labels."""
from soup_cli.utils._eval_text import row_text, tokenize
@ -186,8 +206,8 @@ def build_topic_report(
token_docs = [tokenize(row_text(row)) for row in row_list]
label_terms = ctfidf_labels(token_docs, label_list, k=k, top_n=3)
topics = []
warnings = []
topics: list[Topic] = []
warnings: list[str] = []
for idx in range(k):
members = tuple(
i for i, label in enumerate(label_list) if label == idx