Skip to content

Bound native CUDA cache recovery by measured forward work - #900

Merged
bradhilton merged 6 commits into
mainfrom
schulman/870-bounded-cache-recovery-20260915
Sep 17, 2026
Merged

bradhilton merged 6 commits into
mainfrom
schulman/870-bounded-cache-recovery-20260915

Conversation

@bradhilton

@bradhilton bradhilton commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Unused CUDA cache may not be reusable by the next allocation. This change bases native CUDA admission on physical free memory, tries smaller plans first, and permits cache release when the minimum plan is refused. It reruns planning with fresh counters afterward; release alone never permits execution. Execution failures are not retried.

The first necessary release is allowed and charged. Later releases must fit a measured recovery budget of 5% of completed forward work, including a reserve for the next recovery. Distributed release costs are summed; backward work, idle time and hypothetical savings earn no credit. This is an accounting policy, not a guarantee of total slowdown. Both forward entrypoints share the ledger within a TrainerRank instance.

This standalone change preserves current main's dense context-parallel rank-load pricing, GDN segment costs, planning-error synchronization and original refusal chains. Public APIs, model math and art.megatron are unchanged. It does not depend on #898's pending estimator changes.

Validation:

  • 32 finalized CPU cases on the earlier main integration.
  • Six Account for dense CP rank loads in retained-activation memory #922 boundary cases and two new dense context-parallel recovery cases on the current e8128ba composition.
  • One passing two-worker Gloo test covering seven planning modes, including rank-local retained-token planning failure. Both workers exited successfully.

The tests-only successor repairs stale plan metadata in the distributed fixture, narrows the inert runtime and flat-plan types, and routes only the two dense-CP cases through the Megatron CI stage. The five affected cases pass, all six distributed workers exit zero, and changed-file Ruff/format/type checks pass. A separate Megatron-blocked collection keeps the other 36 cache-recovery cases in the generic stage; collection is not execution.

Hosted two-H200 validation passed parent 40cb87b7; all runtime source bytes are unchanged by this correction. These are focused checks, not a full-suite result. Successor CI and exact-head reviews remain required. The parent's failed type check and the independently reported stale-fixture failures remain recorded.

The retained two-H200 diagnostic passed its four forwards per rank and numerical/state checks, exercising first and earned recovery. Its recovery ledger measured approximately 0.461% of completed forward time; this is not total overhead or net speedup. That run still exited unsuccessfully because rank retirement exceeded its original ten-second bound. Later verified absence does not qualify that failed cleanup or this newer source tree.

Brad explicitly authorized merge on September 17. Subsequent scoped two-H200 evidence passed four forwards per rank, numerical/state checks, initial and earned recovery, and exact-owned cleanup under its original 30-second criterion. Actor retirement required TERM/KILL; this does not erase the earlier diagnostic’s ten-second retirement failure. Three reviewers cleared the merged head and its required CI passed. Backward/optimizer safety, native earned-denial and total training overhead remain separate qualification items; physical re-admission cannot make an underestimated memory demand safe.

@bradhilton
bradhilton had a problem deploying to trainer-rank-gpu-validation September 15, 2026 15:24 — with GitHub Actions Error

@bradhilton bradhilton left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review text removed on 2026-09-17; the consolidated review record is in the summary comment on this PR.

@bradhilton bradhilton left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review text removed on 2026-09-17; the consolidated review record is in the summary comment on this PR.

@bradhilton bradhilton left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review text removed on 2026-09-17; the consolidated review record is in the summary comment on this PR.

@bradhilton bradhilton left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review text removed on 2026-09-17; the consolidated review record is in the summary comment on this PR.

@bradhilton bradhilton left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review text removed on 2026-09-17; the consolidated review record is in the summary comment on this PR.

@bradhilton bradhilton left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review text removed on 2026-09-17; the consolidated review record is in the summary comment on this PR.

Reparent the reviewed runtime and corrected budget fixture onto merged #899.
No tree or runtime changes from a8691cd.
@bradhilton
bradhilton force-pushed the schulman/870-bounded-cache-recovery-20260915 branch from a8691cd to 2b84bc9 Compare September 15, 2026 15:51
@bradhilton
bradhilton changed the base branch from schulman/870-refresh-final-admission-20260915 to main September 15, 2026 15:51
@bradhilton
bradhilton deployed to trainer-rank-gpu-validation September 15, 2026 15:53 — with GitHub Actions Active

@bradhilton bradhilton left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review text removed on 2026-09-17; the consolidated review record is in the summary comment on this PR.

@bradhilton
bradhilton deployed to trainer-rank-gpu-validation September 16, 2026 07:38 — with GitHub Actions Active

@bradhilton bradhilton left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review text removed on 2026-09-17; the consolidated review record is in the summary comment on this PR.

@bradhilton bradhilton left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review text removed on 2026-09-17; the consolidated review record is in the summary comment on this PR.

@bradhilton
bradhilton had a problem deploying to trainer-rank-gpu-validation September 16, 2026 08:25 — with GitHub Actions Error

@bradhilton bradhilton left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review text removed on 2026-09-17; the consolidated review record is in the summary comment on this PR.

@bradhilton bradhilton left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review text removed on 2026-09-17; the consolidated review record is in the summary comment on this PR.

@bradhilton
bradhilton deployed to trainer-rank-gpu-validation September 16, 2026 08:38 — with GitHub Actions Active

@bradhilton bradhilton left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review text removed on 2026-09-17; the consolidated review record is in the summary comment on this PR.

@bradhilton bradhilton left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review text removed on 2026-09-17; the consolidated review record is in the summary comment on this PR.

…ission

Preserve current-main retained-token planning and add portable regression
coverage for exact dense-CP demand after allocator recovery.
@bradhilton
bradhilton deployed to trainer-rank-gpu-validation September 17, 2026 04:43 — with GitHub Actions Active

@bradhilton bradhilton left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review text removed on 2026-09-17; the consolidated review record is in the summary comment on this PR.

@bradhilton bradhilton left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review text removed on 2026-09-17; the consolidated review record is in the summary comment on this PR.

@bradhilton
bradhilton deployed to trainer-rank-gpu-validation September 17, 2026 05:17 — with GitHub Actions Active

@bradhilton bradhilton left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review text removed on 2026-09-17; the consolidated review record is in the summary comment on this PR.

@bradhilton bradhilton left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review text removed on 2026-09-17; the consolidated review record is in the summary comment on this PR.

@bradhilton

Copy link
Copy Markdown
Collaborator Author

Consolidated review record (automated agent review traffic removed 2026-09-17)

Change. Native CUDA admission now uses physical free memory, tries smaller plans first, and permits one bounded cache release when the minimum plan is refused; later releases must fit a 5% budget of measured completed forward work. Public APIs, model math and art.megatron are unchanged.

Review. McCarthy, Minsky, Taravangian and Faraday reviewed eight heads; earlier heads had changes requested. McCarthy, Minsky and Taravangian cleared source correctness at merged head 7480edf with required CI green, including two-H200 validation; Taravangian also cleared the native recovery evidence. All classified it as a significant allocator-policy change requiring Brad's adoption decision.

Findings addressed during review.

  • Split-ladder test fixture let recovery read real CPU availability (four planning calls, not two); recovery budget pinned.
  • Primary exception chain was lost when a local memory sample/release and the following reduction both failed; secondary error now attached as a note.
  • DP-local admission retries re-entered the all-rank checkpoint-slot gather, hanging asymmetric recovery (reproduced on Gloo); slots now ensured once before retrying, with a two-peer regression test.
  • Stale distributed plan double, three ty diagnostics, and dense-CP tests running without Megatron; fixed in tests and workflow.

Deferred / follow-ups.

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.

1 participant