Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/whichllm/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,32 @@ def _validate_output_flags(json_output: bool, markdown_output: bool) -> None:
raise typer.Exit(code=1)


def _validate_ranking_flags(
top: int,
min_speed: float | None,
min_params: float | None,
) -> None:
"""Validate ranking/filter flags that otherwise silently distort output.

Without these guards a non-positive ``--top`` reaches ``results[:top_n]`` in
:func:`whichllm.engine.ranker.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`` /
``--min-params`` thresholds are likewise meaningless. Fail fast with a clear
message instead of producing misleading results.
"""
if top < 1:
console.print("[red]Error:[/] --top must be 1 or greater.")
raise typer.Exit(code=1)
if min_speed is not None and min_speed < 0:
console.print("[red]Error:[/] --min-speed must be 0 or greater.")
raise typer.Exit(code=1)
if min_params is not None and min_params < 0:
console.print("[red]Error:[/] --min-params must be 0 or greater.")
raise typer.Exit(code=1)


def _validate_profile(profile: str) -> str:
"""Validate ranking profile option."""
valid = {"general", "coding", "vision", "math", "any"}
Expand Down Expand Up @@ -539,6 +565,7 @@ def main(

_validate_gpu_flags(cpu_only, gpu, vram, bandwidth, gpu_index)
_validate_output_flags(json_output, markdown_output)
_validate_ranking_flags(top, min_speed, min_params)
profile = _validate_profile(profile)
evidence_mode = _resolve_evidence_mode(evidence, direct)
fit_filter = _resolve_fit_filter(fit, gpu_only)
Expand Down Expand Up @@ -815,6 +842,7 @@ def upgrade(
from whichllm.output.display import display_upgrade, display_upgrade_json

profile = _validate_profile(profile)
_validate_ranking_flags(top, None, None)

with Progress(
SpinnerColumn(),
Expand Down
7 changes: 6 additions & 1 deletion src/whichllm/engine/ranker.py
Original file line number Diff line number Diff line change
Expand Up @@ -877,4 +877,9 @@ def rank_models(
if any(r.estimated_tok_per_sec >= 5.0 for r in results):
results = [r for r in results if r.estimated_tok_per_sec >= 1.5]

return results[:top_n]
# Clamp top_n: a negative value would slice from the end
# (``results[:-5]``) and silently return a truncated, unrequested subset,
# while 0 legitimately yields an empty list. The CLI rejects non-positive
# --top, but guard here too so direct callers of this public helper never
# get a truncated ranking from a stray negative count.
return results[: max(top_n, 0)]
43 changes: 43 additions & 0 deletions tests/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,49 @@ def test_main_json_and_markdown_are_mutually_exclusive():
assert "--json and --markdown are mutually exclusive" in result.stdout


def test_main_top_zero_rejected():
result = CliRunner().invoke(app, ["--top", "0"])

assert result.exit_code == 1
assert "--top must be 1 or greater" in result.stdout


def test_main_top_negative_rejected():
# ``results[:-5]`` would silently drop the best-ranked models.
result = CliRunner().invoke(app, ["--top=-5"])

assert result.exit_code == 1
assert "--top must be 1 or greater" in result.stdout


def test_main_negative_min_speed_rejected():
result = CliRunner().invoke(app, ["--min-speed=-1"])

assert result.exit_code == 1
assert "--min-speed must be 0 or greater" in result.stdout


def test_main_negative_min_params_rejected():
result = CliRunner().invoke(app, ["--min-params=-2"])

assert result.exit_code == 1
assert "--min-params must be 0 or greater" in result.stdout


def test_validate_ranking_flags_accepts_valid_values():
# A valid combination must not raise (no recommendations are dropped).
cli_mod._validate_ranking_flags(top=10, min_speed=0.0, min_params=None)
cli_mod._validate_ranking_flags(top=1, min_speed=None, min_params=7.0)


def test_upgrade_top_zero_rejected():
# The upgrade command shares the same --top -> results[:top_n] hazard.
result = CliRunner().invoke(app, ["upgrade", "RTX 4090", "--top", "0"])

assert result.exit_code == 1
assert "--top must be 1 or greater" in result.stdout


def test_main_empty_gpu_only_result_shows_fit_message(monkeypatch):
captured: dict[str, object] = {}

Expand Down
39 changes: 39 additions & 0 deletions tests/test_ranker.py
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,45 @@ def test_cpu_only_backend_filters_out_non_gguf_models():
assert results[0].model.id == "Qwen/Qwen3-8B-GGUF"


def _gguf_model(model_id: str, family_id: str, downloads: int) -> ModelInfo:
return ModelInfo(
id=model_id,
family_id=family_id,
name=model_id.split("/")[-1],
parameter_count=7_000_000_000,
downloads=downloads,
likes=downloads // 10,
gguf_variants=[
GGUFVariant(
filename=f"{family_id}-Q4_K_M.gguf",
quant_type="Q4_K_M",
file_size_bytes=4_500_000_000,
),
],
)


def test_rank_models_clamps_non_positive_top_n():
# Several distinct families so the full ranking has multiple entries; only
# then is the slice hazard observable.
hw = _make_hardware(vram_gb=24, bandwidth_gbps=300.0)
models = [
_gguf_model("org/Alpha-7B-GGUF", "alpha-7b", 1000),
_gguf_model("org/Beta-7B-GGUF", "beta-7b", 900),
_gguf_model("org/Gamma-7B-GGUF", "gamma-7b", 800),
]

full = rank_models(models, hw, top_n=10)
assert len(full) >= 2 # guard is only meaningful with several results

# 0 and negative requests must yield nothing, never a slice-from-the-end
# subset: ``results[:-1]`` would otherwise return all-but-last.
assert rank_models(models, hw, top_n=0) == []
assert rank_models(models, hw, top_n=-1) == []
# Positive requests are unaffected.
assert len(rank_models(models, hw, top_n=2)) == 2


def test_popularity_has_no_effect_with_direct_benchmark():
model_low_pop = ModelInfo(
id="Qwen/test-8b-lowpop",
Expand Down