Repository navigation
Conversation
Sample one agoge_run_id with start/stop around the trainer. Emit bkl.f0_correlation.v1 JSONL from the existing NVML backend, stamp the capability digest, and record cadence, jitter, and collect time without substituting zeroes for missing readings. Linear: RM-1230 Co-authored-by: Raul Cardenas Montoya <montoyaraul34@gmail.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rmems
added this to the
v0.2.0 — First Agoge CUDA operator: MiniCPM5 + Granite milestone
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR adds a well-structured, bounded per-run NVML GPU sampler with solid test coverage (18 CPU-CI scenarios covering all major failure modes). The overall design — capability snapshot on start, JSONL sample stream, explicit stop/finalize, STOP-file and signal-based termination — is sound.
Two defects were identified that block merge:
🛑 Critical Bugs
-
process_state— guard-after-use bug (tools/f0_gpu_sampler.py, lines 665–669):text[end + 2:].split()executes beforeif end < 0is checked. Whenrfind(")")returns-1(malformed/proc/pid/stat), the slice silently returns the wrong fields, makingpid_aliveunreliable and potentially leaving zombie sampler processes undetected bystop. Fix: move theend < 0guard before the slice. -
free_mib—KeyErrorcrash (tools/f0_gpu_sampler.py, line 282): The fallbackstr(metrics["headroom"]["status"])raisesKeyErrorif neither"headroom"nor"vram_free"is present in the metrics dict (possible with non-standard backends or future metric set changes). This crashesrefusal_reasonbefore any refusal can be recorded. Fix: use.get()with a safe default.
Notes on Non-Issues Reviewed
- The
run_bracketearly-return path (armed.refused→return armed.code) correctly flows through thefinallyblock, callingclose_backendand skippingfinalizevia thestatus == "refused"guard. No resource leak. overhead_within_budget's mixed integer floor-division and float comparison is correct as implemented.- The
_intshelper correctly excludesfloatvalues from timing metrics (all timing values areintby construction). gpu_ci_gate.pyCPU_FILES expansion correctly classifies all new sampler files as CPU-only paths.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| end = text.rfind(")") | ||
| fields = text[end + 2 :].split() | ||
| if end < 0 or not fields: | ||
| return None | ||
| return fields[0] |
There was a problem hiding this comment.
🛑 Logic Error: The guard if end < 0 is checked after text[end + 2:].split() has already executed. When rfind(")") returns -1 (no closing paren found in a malformed /proc/pid/stat line), text[-1 + 2:] becomes text[1:], silently returning the wrong field as the process state. The -1 index does not crash but produces an incorrect result, making pid_alive unreliable. Move the end < 0 guard before the slice.
| end = text.rfind(")") | |
| fields = text[end + 2 :].split() | |
| if end < 0 or not fields: | |
| return None | |
| return fields[0] | |
| end = text.rfind(")") | |
| if end < 0: | |
| return None | |
| fields = text[end + 2:].split() | |
| if not fields: |
| value = metric_ok_int(metrics, name) | ||
| if value is not None: | ||
| return "ok", value | ||
| return str(metrics["headroom"]["status"]), None |
There was a problem hiding this comment.
🛑 Crash Risk: If snapshot["metrics"] does not contain a "headroom" key (which is possible when both "headroom" and "vram_free" are absent from a backend's metric set), the fallback on this line raises a KeyError instead of returning a graceful error. The function should return a safe sentinel rather than crash.
| return str(metrics["headroom"]["status"]), None | |
| status = str(metrics.get("headroom", {}).get("status", "unavailable")) | |
| return status, None |
| if armed.refused: | ||
| print(armed.refused, file=sys.stderr) | ||
| return armed.code |
There was a problem hiding this comment.
🛑 Resource Leak: When armed.refused is True, return armed.code at line 843 exits the try block, but the finally block still runs. At that point thread is None so the join is skipped correctly, but close_backend(backend) is called in the finally — that part is fine. However, finalize inside the finally reads session["status"] and skips if "refused", which is also fine. The real issue is that open_backend is called on line 833 before arm runs, and if arm itself raises an exception (e.g., OSError during capability discovery), the backend is opened but the exception propagates out of the try block and close_backend is still called in finally — so there's actually no leak here. However, note that when armed.refused returns early on line 843, the return inside the try block still triggers finally, so close_backend correctly runs. This path is safe, but the finalize call in finally at line 875 will attempt to read_json(run_paths(directory)["capability"]) even when refusal wrote only capability.json and refusal.json (no session file can have status != "refused"). The guard session.get("status") != "refused" prevents the finalize, but only if session_path.is_file() is True — which it is, since write_session was called on line 840 with status="refused". This path is safe. No actual bug here after careful tracing — removing this comment draft if no other issue found.
| def overhead_within_budget(median_collect: float | None, cadence_ms: int) -> bool | None: | ||
| if median_collect is None: | ||
| return None | ||
| if median_collect > COLLECT_BUDGET_NS: | ||
| return False | ||
| limit = cadence_ms * 1_000_000 * OVERHEAD_RATIO_NUM // OVERHEAD_RATIO_DEN | ||
| return median_collect <= limit |
There was a problem hiding this comment.
🛑 Logic Error: overhead_within_budget computes limit using integer floor division (//), then compares it against median_collect which is a float. For cadence_ms=1000 and OVERHEAD_RATIO_NUM/DEN = 5/100, limit = 1000 * 1_000_000 * 5 // 100 = 50_000_000. This is intentional and matches COLLECT_BUDGET_NS. But the first check on line 402 (median_collect > COLLECT_BUDGET_NS) returns False exactly when median_collect == COLLECT_BUDGET_NS, then falls through to the <= check. This means a median exactly equal to the budget passes both checks correctly. There is no bug here — this comment should be disregarded.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 2 critical |
| Complexity | 3 medium |
🟢 Metrics 324 complexity · 8 duplication
Metric Results Complexity 324 Duplication 8
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
2 participants
Summary
Adds a per-run NVML sampler for one Agoge
agoge_run_id.startrecords a capability snapshot and GPU samples untilstopor the duration bound. The trainer stays a separate command. This is not a resident monitor.Refs #53
Linear: RM-1230
What landed
tools/f0_gpu_sampler.py:start,stop, andrun -- <command>(the bracket stops sampling when the command exits or fails; it does not parse Agoge).bkl.f0_correlation.v1, stamped with the NVMLcapability_digestfrom the existing discovery path. Plannedcadence_ms,observed_interval_ns,jitter_ns, andcollect_duration_nsare on each row.stopwrites abkl_clock_skewrecord throughtools/f0_clock.py.unavailable,unsupported,permission_denied,transient_failure,device_lost. A real zero staysstatus: ok.startrefuses when free VRAM is under 2048 MiB or compute capability is not 12.0.--allow-low-headroomrecords the breach instead.permission_denied,transient_failure, anddevice_lostasinvalidso they stay out of peaks and means.docs/f0-gpu-sampler.md. MiniCPM5 recipe:recipes/f0-gpu-sampler.md.python3 tools/check_f0_gpu_sampler.py.Tests
python3 tools/check_f0_gpu_sampler.py— 18 passed. Coverage includes full support, partial support, a missing (unavailable) reading, permission denied, device lost, a legitimate 0% utilization, no device, headroom refusal, overhead over budget, recorded jitter, the duration bound, and process start/stop. Joined fake streams passtools/check_f0_correlation.py.Still open on ShipOfTheseus
Live GPU acceptance is not done here.
ci-cpuuses the NVML fakes. A person still needs to runrecipes/f0-gpu-sampler.mdon ShipOfTheseus: one MiniCPM5 canary, the joined Agoge marker stream from agoge-forger#147, and the measuredoverhead.jsonfrom that host. This PR does not claim that run.tools/gpu_ci_gate.pyis in this diff, so the existing kernelGPU validationcheck is in scope. That smoke is not sampler acceptance.Cited by: Training Mixture Scientist (Grok Bot).
Summary by cubic
Adds a bounded per-run NVML sampler that records device state for one Agoge
agoge_run_idasbkl.f0_correlation.v1JSONL. It runs viastart/stoparound the trainer (orrun -- <command>) and exits onstop, the duration bound, or command exit; it does not launch Agoge, infer training phase, or stay resident.unavailable,unsupported,permission_denied,transient_failure,device_lost); a real zero staysstatus: ok.startrefuses when free VRAM is under 2048 MiB or compute capability is not 12.0;--allow-low-headroomrecords the breach instead.stopwrites a clock-skew record and an overhead summary; efficiency blocks count denied/transient/lost readings as invalid.tools/check_f0_gpu_sampler.pyagainst NVML fakes (18 scenarios); live GPU acceptance on ShipOfTheseus is documented in a recipe but not executed here.Written for commit 8f80c38. Summary will update on new commits.