Repository navigation
perf(optim): replay CUDA update preparation - #19
Conversation
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (19 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit e5d7a85)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit e5d7a85)Status: No Issues Found | Recommendation: Merge Files Reviewed (13 files)
Reviewed by free · Input: 599.2K · Output: 74.1K · Cached: 4.4M |
There was a problem hiding this comment.
🔵 Needs a closer look
Capturing live optimizer momentum needs human confirmation of accelerator validation, with the full gate and accelerator goldens not yet run.
No open issues 🎉
What changed in this PR
Adds opt-in CUDA replay of MuSGD update preparation to reduce optimizer overhead while keeping learning-rate application and parameter writes outside capture.
Changes:
- Adds group-level replay with state restoration, compatibility guards and reload invalidation.
- Exposes
capture_updatesthrough the training module and adds regression coverage. - Documents measured performance, memory costs and validation limits.
No actionable findings remain. Full gate and accelerator-golden validation remain outstanding.
| File | Description |
|---|---|
| tests/ptl/test_cli.py | Tests flag forwarding to the optimizer. |
| tests/optim/test_update_replay.py | Tests replay parity, fallbacks and state handling. |
| src/lucid_yolo/ptl/module.py | Exposes and forwards the replay option. |
| src/lucid_yolo/optim/update_replay.py | Implements guarded capture and replay. |
| src/lucid_yolo/optim/musgd.py | Integrates replay and extracts native update preparation. |
| docs/work_packages/wp-197c.md | Records design, measurements and validation limits. |
| docs/work_packages/wp-197b.md | Links the follow-up work package. |
| docs/TRAINING.md | Documents the opt-in training flag. |
| docs/ROADMAP.md | Tracks the open replay work package. |
| docs/PROVENANCE.md | Records update-replay API provenance. |
| docs/engineering/phase-17-pose-objective-parity.md | Includes the new work-package record. |
| docs/DECISIONS.md | Extends the CUDA replay decision. |
| CHANGELOG.md | Announces optional update-preparation replay. |
Review effort: Balanced
Replay the existing MuSGD preparation and momentum work while learning-rate application and parameter writes remain outside capture. Keep unsupported signatures native and recordings bounded to the optimizer's current state. WP: 197 Provenance: R42; existing optimizer expressions and recorded CUDA measurements Assumptions: none Gate: tests/optim/test_update_replay.py; tests/ptl/test_cli.py::test_capture_updates_flag_reaches_optimizer --- Signed-off-by: Jesús Royeth <JESUSROYETH@users.noreply.github.com>
Merge current main, keeping the network and optimizer replay options separate and default-off. Append the update option after existing positional arguments. Check all eight CLI flag combinations and optimizer forwarding. Preserve the original optimizer and replay implementation. WP: 197 Provenance: R42, R43; existing repository implementations and API contracts Assumptions: none Gate: tests/ptl/test_cli.py::test_capture_updates_flag_reaches_optimizer; tests/optim/test_update_replay.py --- Signed-off-by: Jesús Royeth <JESUSROYETH@users.noreply.github.com>
Merge the input preparation and prepared-cache changes now on main. Retain all work-package and source records and regenerate their navigation links. Keep the optimizer implementation and launch flags unchanged. WP: 197 Provenance: R42, R43, R44; existing repository implementations and records Assumptions: none Gate: tests/optim/test_update_replay.py; tests/ptl/test_cli.py; tests/ptl/test_datamodule.py --- Signed-off-by: Jesús Royeth <JESUSROYETH@users.noreply.github.com>
e5d7a85 to
623edc0
Compare
WP_RE now accepts a roadmap row id of digits with one optional lowercase letter, such as 197c, alongside plain digits and none, so a lettered row no longer has to be cited as its numeric sibling. The violation message names the new 'WP: <digits>[letter]' form, and new tests cover lettered ids. WP: 197c Provenance: none (review remediation; commit-trailer tooling) Assumptions: none Gate: scripts/_tests/test_check_commit_trailers.py --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> Signed-off-by: jirka <6035284+borda@users.noreply.github.com>
After _MISMATCH_TOLERANCE (3) consecutive signature mismatches a group synchronizes its replay stream, releases the recording and records the new signature, up to _RERECORD_BUDGET (2) replacements, after which it stays native until clear(). Each step compares the recording key first; the full input scan runs only when recording, while every step still checks strided layouts, the lr type and gradient spans against the recorded parameter and momentum spans. Orthogonalization and update replays build their runtime key from one shared helper in replay.py. A recorded update whose strides differ from its parameter is copied into the parameter's strides inside the capture. CHANGELOG and TRAINING state that a failure while recording raises after gradients and momentum are restored. New CPU and CUDA tests cover the input guards, each key component, private gradient copies and recorded stride matching. WP: 197c Provenance: none (review remediation; implementation efficiency) Assumptions: none Gate: tests/optim/test_update_replay_guards.py --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> Signed-off-by: jirka <6035284+borda@users.noreply.github.com>
The capture_updates entry lists its own native-fallback guards instead of claiming those of capture_orthogonalization, describes when a group replaces its recording, says a failure while recording raises after gradients and momentum are restored, and says a MuSGD subclass always steps natively. New CPU tests step deepcopied and pickled optimizers with capture_updates and check that load_state_dict clears recordings. WP: 197c Provenance: none (review remediation; documentation and test coverage) Assumptions: none Gate: tests/optim/test_musgd_update_copies.py; src/lucid_yolo/optim/musgd.py doctests --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> Signed-off-by: jirka <6035284+borda@users.noreply.github.com>
New tests cover a failure inside capture and in the first replay run, groups with their own rates, a group added after recording, stacked equal-shape matrices, a MuSGD subclass keeping the native route, and non-finite gradients fed straight to the replay. GPU reference twins set capture_updates=False explicitly, and test_capture_updates_default_off pins the CLI default. The float ns_steps test is renamed test_float_ns_steps_refused_not_cached_as_int. The work-package log says the gpu-marked CUDA class runs only on a CUDA host. WP: 197c Provenance: none (review remediation; test coverage) Assumptions: none Gate: tests/optim/test_update_replay.py; tests/ptl/test_cli.py::test_capture_updates_default_off --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> Signed-off-by: jirka <6035284+borda@users.noreply.github.com>
The 197c row states the regime behind its fit figure and names its test ids in the gate cell. D29 carries a dated amendment marker for 197c and limits the momentum-outside-graphs sentence to capture_orthogonalization alone. WP: 197c Provenance: none (review remediation; documentation) Assumptions: none Gate: pre-commit docs hooks (mdformat, wp-links, docs-presence) --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> Signed-off-by: jirka <6035284+borda@users.noreply.github.com>
Exercise signature rejection, recording rollback and cache lifetime with real CPU tensor storage and stubbed CUDA submission. Check independent returned storage and retain the existing real-CUDA trajectories for device semantics. WP: 197 Provenance: R42; existing repository replay and optimizer contracts Assumptions: none Gate: tests/optim/test_replay_state.py --- Signed-off-by: Jesús Royeth <JESUSROYETH@users.noreply.github.com>
…ion-20261018' into codex/lucid-update-preparation-20261018 Signed-off-by: jirka <6035284+borda@users.noreply.github.com>
The stubbed update recordings in test_replay_state.py now carry replay_stream, spans and mismatches, which the update replay reads on each step and on release, so the cache-ownership and failed-submission cases exercise the current owner instead of failing on missing attributes. Assertions are unchanged. WP: 197c Provenance: none (merge follow-up; test stubs) Assumptions: none Gate: tests/optim/test_replay_state.py --- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> Signed-off-by: jirka <6035284+borda@users.noreply.github.com>
| buffers: list[Tensor], | ||
| ) -> bool: | ||
| """Record and run the group's current signature unless its budget is spent or its inputs are unsupported.""" | ||
| if self._exhausted(group) or not self._supported(params, gradients, buffers, group): |
There was a problem hiding this comment.
WARNING: The CHANGELOG and docstring both promise that "a failure while recording raises (gradients and momentum are restored first)". _record_group (lines 238-248) calls _PreparedGroup(...) and entry.run(...) outside any try/finally, and _PreparedGroup._record restores gradients and momentum only in its own finally (lines 142-150). So a failure during the second recording pass (the torch.cuda.graph block at line 137) does restore gradients and momentum, but a failure during the first warmup pass (line 133, inside with torch.cuda.stream(self.stream)) restores nothing, because that block is not wrapped. The CHANGELOG line 11 and the capture_updates docstring overstate the guarantee. Either the warmup pass needs the same restore, or the prose needs qualifying.
|
|
||
| [MEASURED] Complete-fit time falls **10.01%**; paired reductions span 9.62–10.33%. Every model, learned optimizer, scheduler, EMA, loss-sequence and validation-metric byte hash agrees within pairs; independent baselines agree too. Only the execution flag is excluded from optimizer metadata and recorded separately. Actual state tensors from a preselected pair are saved and independently re-hashed. This is throughput and numerical-trajectory evidence, not convergence or accuracy. Added memory is a cost, not a capacity benefit; one batch-32 screen showed no benefit. | ||
|
|
||
| **Checks and scope.** [MEASURED] 169 focused CPU/CUDA/doctest checks pass with 4 expected skips on PyTorch 2.9.1; 165 pass with 8 skips on the exact 2.4.0 floor. Layout, inference-guard and scalar-key mutations fail the corresponding regressions. The CUDA class in `tests/optim/test_update_replay.py` (every replay byte-parity, reload, AMP, layout, alias and signed-zero check) is marked `gpu` and skipped without CUDA, and `make test` deselects `gpu`: those checks run only on a CUDA host, and CI certifies the CPU-native route, the reload flag and the CLI wiring alone until `gate-gpu.yml` has an armed runner. Optimizer sources are byte-identical between `e5d7a85` and `623edc0`, so a CUDA run recorded against either commit covers both. Full precommit and strict mypy pass. The full development gate and accelerator goldens were not run, so the row stays open. Other tasks, models, accelerators, DDP and long convergence carry no performance claim. R42 governs the CUDA API; all preparation expressions come from this repository and our own profiles. |
There was a problem hiding this comment.
WARNING: The claim "Optimizer sources are byte-identical between e5d7a85 and 623edc0, so a CUDA run recorded against either commit covers both" is stale. Commits after 623edc0 in this range — 1b8e9fe (fix(optim): re-record replay after lasting change) and 42bfdb9 — rewrite src/lucid_yolo/optim/{musgd,replay,update_replay}.py (206 insertions / 58 deletions over 623edc0..HEAD), so neither named commit matches the tree at HEAD and the CUDA evidence this sentence is meant to transfer no longer covers the current optimizer. The "169 focused CPU/CUDA/doctest checks" figure likewise predates the new test files added after 623edc0 (tests/optim/test_update_replay_guards.py, tests/optim/test_replay_state.py).
| | 2026-10-16 | R43 | First read of the PyTorch 2.4 CUDA graphed-callable contract and exact-tag licence for [WP-193c](work_packages/wp-193c.md); network arithmetic comes from this repository and the opportunity from its own L4 profiles. | | ||
| | 2026-10-17 | R44 | Optional compiler for the existing colour expressions in [WP-198](work_packages/wp-198.md); own CUDA profiles, launch-shape measurements and public-pixel comparisons select the fusion. | | ||
| | 2026-10-17 | R40 | The existing runtime's [persistent DataLoader iterator](https://github.com/pytorch/pytorch/blob/v2.9.1/torch/utils/data/dataloader.py#L481-L494) is read diagnostically for [WP-198](work_packages/wp-198.md); the same reset is scheduled earlier, with local RNG/worker and end-to-end evidence. No framework implementation copied. | | ||
| | 2026-10-18 | R42 | Update-preparation scheduling for [WP-197c](work_packages/wp-197c.md), based on this repository's optimizer and our L4 traces; stable buffers, warmup rollback and non-nested capture use the existing API contract. No external optimizer or detector implementation consulted. | |
There was a problem hiding this comment.
WARNING: This is a second usage-log row for R40 (the first is at line 142, dated 2026-10-16), but the table's own preamble (line 120) states it is a first-use log where "a source appears once, on the day it was first read, and every later use of it is traced through the Provenance: trailers ... rather than by another line here." Either the preamble rule or this row must go. Note the pre-existing 2026-10-18 R42 row (line 148) has the same shape, so the rule and the practice need reconciling once.
The opt-in flag
--model.capture_updates truecuts complete nano detection fit time by 10.01% on the small-batch CUDA workload I measured it on. Five paired ten-epoch runs on public COCO (256 training images, 32 disjoint validation images, 640 px, batch 4, BF16) take 120.850 (120.542–122.793) s native and 108.750 (108.123–110.967) s with replay.MuSGD records its existing update preparation once, momentum included, and replays it on each new gradient, copied into owned inputs. I kept the learning-rate reads and the parameter writes outside the capture. The update layouts match, so the final foreach write avoids individual add_ calls. Both benchmark arms already had the merged orthogonalization replay, decoded-image caching and device HSV on, and the timing covers the whole fit: setup, loading, augmentation, backward, optimizer, EMA, validation, checkpoints. Measured on an NVIDIA L4, driver 580.178.04, PyTorch 2.9.1+cu129, Lightning 2.6.6.
Model, learned optimizer state, scheduler, EMA, loss and metric byte hashes all match. Peak allocated memory does go up though, from 877.8 to 924.6 MiB, so the default stays off. Unsupported inputs fall back to the native path, warmup restores the state correctly, and reload clears the recordings. Scalar keys keep their native types, and the floating-point bits stay untouched.
Focused coverage passes on PyTorch 2.4.0 and 2.9.1 — inference, reload, AMP and scalar-setting regressions, plus three mutation checks, strict mypy and the full precommit run. I did not run the full
make gateor the accelerator goldens. One batch-32 screen showed no benefit, so there's no performance claim for other workloads or for long-run accuracy.Clean-room acknowledged: only unchanged repository expressions, our own L4 profiles and the registered PyTorch CUDA API went into this — no external optimizer implementation consulted.
WP: 197
Provenance: R42 is the CUDA API contract; unchanged update expressions come from this repository
Assumptions: none
Gate: tests/optim/test_update_replay.py; tests/ptl/test_cli.py::test_capture_updates_flag_reaches_optimizer