Search test coverage, submit PR flow, scoring consolidation - #1
Merged
Conversation
Change 1 — real coverage for the tuning search (the core, previously untested): - Extract run_tune's inline VRAM detection into _detect_vram_mib() (removes a near-duplicate of detect._detect_gpu and gives tests a monkeypatchable seam) - Move MockBenchRunner to tests/conftest.py; new tests/test_tune.py drives run_tune fully offline, covering coarse sweep, hill climb, trial budget, dedup, early convergence, and MoE/dense ik-flag paths Change 2 — kill the scoring-duplication bug: - Single canonical types.score_trial(); TrialResult.score and tune._score_trial both delegate, so custom --pp-weight/--tg-weight are honored everywhere Change 3 — build the real `dyno submit` PR flow (PR → Gist → local): - submit_via_pr forks+clones the community repo, commits the result under results/<gpu>/, pushes, and opens a PR via gh; falls back cleanly - tests/test_submit.py covers the full fallback chain with gh/git mocked - Fix the submit CLI docstring Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Improves dyno’s quality and UX by adding offline integration tests for the tuning search, consolidating trial scoring into a single canonical function, and implementing the documented dyno submit PR-first submission flow with fallbacks.
Changes:
- Add offline
run_tuneintegration tests using a sharedMockBenchRunnerand monkeypatch seams (incl. VRAM detection). - Consolidate scoring logic into
types.score_trial()and route bothTrialResult.scoreand the tuner’s scoring through it. - Implement PR → Gist → local submission chain for
dyno submit, with tests covering the fallback behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_tune.py | New offline integration tests covering run_tune phases and behaviors. |
| tests/test_submit.py | New tests covering submit PR-first flow and fallbacks with mocked gh/git. |
| tests/test_search.py | Refactors to use shared MockBenchRunner and retains unit tests around types/search helpers. |
| tests/conftest.py | Adds shared MockBenchRunner test helper. |
| src/llama_dyno/types.py | Adds canonical score_trial() and updates TrialResult.score to delegate. |
| src/llama_dyno/tune.py | Delegates scoring to score_trial() and extracts VRAM detection into _detect_vram_mib(). |
| src/llama_dyno/submit.py | Adds PR-based submission path and updates submit fallback chain/message. |
| src/llama_dyno/cli.py | Updates submit command docstring to match new behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| from __future__ import annotations | ||
|
|
||
| from conftest import MockBenchRunner |
| The mock returns speeds that vary by parameters, allowing us to verify | ||
| the tuner picks the optimal config. | ||
| """ | ||
| from conftest import MockBenchRunner |
Comment on lines
+404
to
+412
| try: | ||
| import pynvml | ||
| pynvml.nvmlInit() | ||
| handle = pynvml.nvmlDeviceGetHandleByIndex(0) | ||
| vram = pynvml.nvmlDeviceGetMemoryInfo(handle).total // (1024 * 1024) | ||
| pynvml.nvmlShutdown() | ||
| return int(vram) | ||
| except Exception: | ||
| pass |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Raises code quality per the approved plan — targets the three things capping the rating.
Change 1 — Real test coverage for the tuning search (the big one)
The search algorithm (
run_tune/_coarse_sweep_ngl/_hill_climb, ~450 lines) previously had zero real coverage —MockBenchRunnerwas never wired intorun_tune.run_tune's inline VRAM detection into_detect_vram_mib()— removes a near-duplicate ofdetect._detect_gpuand gives tests a monkeypatchable seam.MockBenchRunnertotests/conftest.py; newtests/test_tune.pydrivesrun_tunefully offline, asserting: optimal-config selection, OOM rejection, trial-budget caps, no duplicate benches, early-convergence skip, and MoE-vs-dense ik-flag behavior.Change 2 — Kill the scoring-duplication bug
TrialResult.scorehardcoded0.3/0.7, diverging from_score_trial's configurable weights (so--pp-weight/--tg-weightwere silently ignored by the property). Now one canonicaltypes.score_trial(); both callers delegate.Change 3 — Build the real
dyno submitPR flowREADME promised PR → Gist → local; code only did Gist → local.
submit_via_pr: forks+clones the community repo, commits the result underresults/<gpu>/, pushes, opens a PR viagh; falls back cleanly on any failure.tests/test_submit.pycovers the whole fallback chain withgh/gitmocked (no network).submitCLI docstring.Tests
34 passing locally (was 22). No behavioral change to
detect/bench/report.🤖 Generated with Claude Code