Skip to content

fix(cli): validate ranking flags to avoid silently wrong output#142

Open
SuperMarioYL wants to merge 1 commit into
Andyyyy64:mainfrom
SuperMarioYL:fix-validate-ranking-flags
Open

fix(cli): validate ranking flags to avoid silently wrong output#142
SuperMarioYL wants to merge 1 commit into
Andyyyy64:mainfrom
SuperMarioYL:fix-validate-ranking-flags

Conversation

@SuperMarioYL

Copy link
Copy Markdown
Contributor

What

The ranking/filter flags --top, --min-speed, and --min-params were never validated, so invalid values silently produced misleading output instead of a clear error — unlike --vram, --bandwidth, and --gpu-index, which already fail fast.

The motivating case is --top, which flows straight into results[:top_n] in rank_models:

  • --top 0results[:0]no recommendations are shown at all, with no explanation.
  • --top -5results[:-5] → slices from the end, silently returning a truncated subset (the lowest-ranked entries dropped) instead of the count the user asked for.

Negative --min-speed / --min-params thresholds are meaningless (they turn the filter into a no-op), so they are rejected too.

Changes

  • Add _validate_ranking_flags() in cli.py (--top ≥ 1, --min-speed ≥ 0, --min-params ≥ 0), following the existing _validate_gpu_flags style, and wire it into both the default recommend flow and the upgrade command (which also exposes --top).
  • Defense in depth: clamp top_n in rank_models to results[: max(top_n, 0)] so direct callers of this public helper never get a truncated ranking from a stray negative count.
  • Tests for each rejected flag and the clamp behavior.

Notes

  • --context-length already raises on non-positive values via parse_context_length, so it is intentionally left unchanged.
  • ruff check and ruff format --check are clean; the full test suite passes locally (the only failure is the pre-existing test_run_exits_gracefully smoke test, which needs a working uv/network and fails identically on an unmodified checkout — unrelated to this change).

A non-positive --top reaches results[:top_n] in rank_models: --top 0
returns no recommendations at all, and a negative value slices from the
end (results[:-5]), silently returning a truncated, unrequested subset
instead of the count the user asked for. Negative --min-speed and
--min-params thresholds are likewise meaningless.

Add _validate_ranking_flags to fail fast with a clear error (mirroring
the existing --vram / --gpu-index guards), wire it into the recommend
and upgrade commands, and clamp top_n in rank_models so direct callers
of the public helper never get a truncated ranking from a stray
negative count.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant