Skip to content

refactor(display): split output/display.py by output surface (#41)#82

Open
RaghavRD wants to merge 4 commits into
Andyyyy64:mainfrom
RaghavRD:refactor/display-split
Open

refactor(display): split output/display.py by output surface (#41)#82
RaghavRD wants to merge 4 commits into
Andyyyy64:mainfrom
RaghavRD:refactor/display-split

Conversation

@RaghavRD

@RaghavRD RaghavRD commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Move the 880-line output/display.py into focused per-surface modules:

  • output/_console.py — canonical Console() instance
  • output/formatting.py — byte / param / date / color helpers
  • output/ranking.py — display_ranking + display_hardware
  • output/plan.py — display_plan
  • output/upgrade.py — display_upgrade + summarize/verdict helpers
  • output/json_output.py — display_json + display_plan_json + display_upgrade_json
  • output/display.py — thin re-export shim (38 lines, was 880)

Tests that captured output by patching display.console now patch whichllm.output._console.console instead, which is the new single source of truth that every surface module looks up at call time.

Behavior-preserving: no ranking, plan, upgrade, or JSON output changes. All 220 tests pass.

Refs #41

What

Splits the 880-line src/whichllm/output/display.py into focused per-surface modules:

Module Purpose Lines
output/_console.py Canonical Console() instance 11
output/formatting.py byte / param / date / color helpers 101
output/ranking.py display_ranking + display_hardware + ranking confidence helper 331
output/plan.py display_plan 158
output/upgrade.py display_upgrade + _summarize_row + _upgrade_verdict 124
output/json_output.py display_json + display_plan_json + display_upgrade_json 200
output/display.py Thin re-export shim 38 (was 880)

output/display.py becomes a thin shim that re-exports every display_* public function plus console, so every existing from whichllm.output.display import … import keeps working with no churn in cli.py or anywhere else.

Test monkey-patch update

Three tests captured Rich output by reassigning whichllm.output.display.console = Console(file=buf, …). After the split, that attribute is only the shim's local binding and doesn't reach the per-surface modules. The tests now target the new single source of truth, whichllm.output._console.console, which every surface looks up at call time. Same capture semantics, three-line change per site:

  • tests/test_cli.py (2 sites, plan-JSON and ranking-JSON capture)
  • tests/test_intel_gpu.py (Intel shared-memory hardware display)
  • tests/test_amd_detection.py (AMD shared-memory hardware display)

Why

Per #41, display.py mixed five output surfaces (ranking, plan, upgrade, JSON, shared formatting), each with its own change cadence. Splitting them means future tweaks to the upgrade verdict, the plan VRAM table, or the JSON schema stay local to one file.

Behavior

Strictly behavior-preserving:

  • No ranking output changes
  • No plan output changes
  • No upgrade output changes
  • No JSON schema changes
  • Public import surface preserved via the shim
  • All 220 existing tests pass

Test plan

  • uv run pytest — 220 passed
  • uv run whichllm --help — works
  • Shim re-exports: from whichllm.output.display import display_hardware, display_ranking, display_plan, display_plan_json, display_upgrade, display_upgrade_json, display_json, console — all resolve

Next in series

After this lands, per the agreed plan in #41: PR 4 will split models/fetcher.py.

RaghavRD added 3 commits June 3, 2026 23:48
Move the 880-line output/display.py into focused per-surface modules:

- output/_console.py        — canonical Console() instance
- output/formatting.py      — byte / param / date / color helpers
- output/ranking.py         — display_ranking + display_hardware
- output/plan.py            — display_plan
- output/upgrade.py         — display_upgrade + summarize/verdict helpers
- output/json_output.py     — display_json + display_plan_json + display_upgrade_json
- output/display.py         — thin re-export shim (38 lines, was 880)

Tests that captured output by patching display.console now patch
whichllm.output._console.console instead, which is the new single source
of truth that every surface module looks up at call time.

Behavior-preserving: no ranking, plan, upgrade, or JSON output changes.
All 220 tests pass.

Refs Andyyyy64#41
Resolves a conflict in src/whichllm/output/display.py introduced by upstream b81497d (fix: apply small GPU and display fixes), which modified display_upgrade in the pre-split monolithic display.py.

Since display.py is now a thin re-export shim and display_upgrade lives in output/upgrade.py, the equivalent change has been ported there: treat vram_gb == 0.0 as 0 GB rather than dash by checking 'is not None' instead of truthiness.

All 228 tests pass; ruff check and ruff format clean.
@RaghavRD

RaghavRD commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

Brought the branch up to date with upstream/main. The only conflict was in src/whichllm/output/display.py, caused by #66's narrowed fix touching display_upgrade in the pre-split monolith. Since display.py is now a re-export shim and display_upgrade lives in output/upgrade.py, I ported the equivalent change there: switched the vram_gb check from truthiness to is not None so 0 GB shared-memory rows render as "0 GB" rather than "—".

All tests still pass locally
ruff check + format are clean.

@Andyyyy64

Copy link
Copy Markdown
Owner

I reviewed this properly today, and the split itself is in good shape. The _console late-lookup pattern is the right call (surface modules read _console.console at call time, so test capture keeps working), the shim keeps every existing import alive, and the suite passes on your branch as-is.

It now conflicts with main: #68 landed changes inside display_hardware, which your branch moved into output/ranking.py. Could you rebase? Three concrete things to carry over:

  1. Resolve output/display.py in favor of your shim.
  2. Port the display_hardware fixes from 721ce3b into output/ranking.py: drop the extra.append("shared memory") block for discrete AMD/Intel cards, and stop showing shared memory for amd/intel cards with vram_bytes == 0 unless shared_memory is actually set.
  3. fix: correct AMD discrete GPU detection on Linux #68 also added display tests in tests/test_amd_detection.py and tests/test_intel_gpu.py that monkeypatch display.console. Under your split they need to patch whichllm.output._console.console instead, same as you did for the three older tests.

After that I'm happy to merge. The upcoming table-UX work will build on these modules, so I'd like to land this first.

Resolves a conflict in src/whichllm/output/display.py from upstream 721ce3b (fix: correct AMD discrete GPU detection on Linux, Andyyyy64#68), which removed two heuristics inside display_hardware in the pre-split monolithic display.py.

Since display.py is now a re-export shim and display_hardware lives in output/ranking.py, the equivalent changes have been ported there: simplified the non-shared GPU vram branch to a plain _format_bytes call, and removed the inverted 'extra: shared memory' tag that was incorrectly added to non-shared AMD/Intel GPUs with VRAM > 0.

Also updates two new test_amd_detection.py tests added by Andyyyy64#68 to monkey-patch whichllm.output._console.console (the new single source of truth) instead of display.console. With the old pattern they would have either failed outright (one did) or passed for the wrong reason because the buf they wrote into stayed empty.

All 329 tests pass; ruff check and ruff format clean.
@RaghavRD

Copy link
Copy Markdown
Contributor Author

@Andyyyy64 thanks for the review! Brought the branch up to date with upstream/main. The only conflict was again in src/whichllm/output/display.py, this time from #68's two display_hardware cleanups. Since display.py is a re-export shim and display_hardware lives in output/ranking.py, I ported the equivalent changes there:

  • Simplified the non-shared vram branch to a plain _format_bytes(gpu.vram_bytes) call.
  • Removed the inverted "shared memory" extra-tag that was being added to non-shared AMD/Intel GPUs with VRAM > 0.

While porting, I also caught two new test_amd_detection.py tests added by #68 that were still monkey-patching display.console (the pre-split single-module location). One was failing outright; the other was passing for the wrong reason — its not in output assertion was vacuously true because the buf stayed empty. Both now monkey-patch whichllm.output._console.console to match the other tests in the file, so they actually validate what they claim to validate.

test_intel_gpu.py and the other monkey-patches were already on the new pattern from the previous round.

All 329 tests pass locally; ruff check + format are clean. Ready for another look whenever you have a moment.

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