Skip to content

refactor(cli): split command helpers into focused modules - #162

Open
RaghavRD wants to merge 1 commit into
Andyyyy64:mainfrom
RaghavRD:refactor/cli-split
Open

refactor(cli): split command helpers into focused modules#162
RaghavRD wants to merge 1 commit into
Andyyyy64:mainfrom
RaghavRD:refactor/cli-split

Conversation

@RaghavRD

Copy link
Copy Markdown
Contributor

No worries at all, and thanks for taking the time to review it.

I really appreciate you retesting it across Python 3.11, 3.12, and 3.13 and updating it against current main. I’ve opened the final #41 refactor slice for src/whichllm/cli.py as follows

Move command bodies, validation, hardware overrides, model lookup, runtime script generation, and shared CLI utilities into focused modules.

Keep whichllm.cli as the Typer entry point and compatibility shim so existing public and test imports continue to work.

What

  • Move command execution bodies into whichllm.cli_commands
  • Move model loading, search, GGUF selection, dependency resolution, and chat script generation into whichllm.cli_models
  • Move shared console, async runner, version, and fetch-error helpers into whichllm.cli_shared
  • Move validation, memory budgets, GPU overrides, ranking filters, and publish-date backfill helpers into whichllm.cli_validation
  • Keep command signatures and old helper imports available from whichllm.cli

Why

This completes the remaining large core-module split for #41. After the previous constants, display, fetcher, benchmark, and ranker splits, src/whichllm/cli.py was the last large core module left.

Testing

  • CLI tests pass (95 passed)
  • Full test suite passes (484 passed)
  • Compileall passes
  • Ruff check passes
  • Ruff format check passes

Notes

uv run is currently blocked locally because this repo now requires uv ==0.11.33 while my local uv is 0.11.17, so I ran verification through the existing .venv:

  • .venv/bin/python -m compileall -q src/whichllm
  • .venv/bin/python -m pytest -q tests/test_cli.py
  • .venv/bin/python -m pytest -q
  • .venv/bin/python -m ruff check .
  • .venv/bin/python -m ruff format --check .

Move command bodies, validation, hardware overrides, model lookup, runtime script generation, and shared CLI utilities into focused modules.

Keep whichllm.cli as the Typer entry point and compatibility shim so existing public and test imports continue to work.

Complete the remaining large core-module split for Andyyyy64#41.
@MohammedAlkindi

Copy link
Copy Markdown

Ran the branch rather than read it. The split is behavior-preserving in the executable sense, with one thing worth restoring before merge.

Setup: clean clone, Windows 11, Python 3.14.7, pip install -e ., merge-base ea32ed2 (== current main), PR head e0f66ed.

  • Full suite, both sides: 3 failed, 481 passed at the merge-base and 3 failed, 481 passed at e0f66ed, and the failing sets are identical — all three are tests/test_asahi_detection.py symlink-privilege failures (WinError 1314) that fail the same way on main, i.e. my environment, not this PR. No new failures.
  • Import shim holds: everything tests/test_cli.py imports from whichllm.cli still resolves (95 passed), and all four new modules import clean. The whichllm.cli namespace no longer re-exports incidental imports (sys, re, asyncio, Console, HardwareInfo, …); nothing in the repo reaches through those, so nothing breaks.
  • Moves: comparing all 37 functions that left cli.py against their merge-base source: none missing, 20 byte-identical, 6 command bodies deliberately restructured into the keyword-only *_command form (decorators staying in cli.py), and 11 helpers whose only difference is trimmed docstrings/comments.
  • CLI surface: --help for the top level and for plan/run/snippet/hardware is identical on both sides. upgrade --help is not:
 --- merge-base
 +++ e0f66ed
-  For each GPU passed on the command line, simulate a system with the same
-  CPU/RAM but that GPU, run the ranker, and show the best-N models you'd
-  be able to run. Useful for answering "is upgrading from a 3090 to a 4090
-  worth it?" — the table shows the quality jump and the speed jump for
-  each option.

That last point is the one ask: the move trimmed several multi-paragraph rationale docstrings (_merge_model_eval_benchmarks's deprecation history, _auto_min_params_for_profile, _parse_size_tokens, …) and the upgrade command's docstring — and Typer renders that one as the help text, so the deletion is user-visible. The code moves themselves look right; restoring the docstrings, or at minimum upgrade's, would make this a pure move.

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.

2 participants