Skip to content

Reject unsupported selective recompute in TrainerRank planning - #914

Merged
bradhilton merged 1 commit into
mainfrom
fix/913-reject-selective-recompute
Sep 16, 2026
Merged

bradhilton merged 1 commit into
mainfrom
fix/913-reject-selective-recompute

Conversation

@bradhilton

@bradhilton bradhilton commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

A selective-recompute runtime can pass TrainerRank's activation-memory admission check and then OOM on its first forward: the current static estimate is calibrated for full recompute. Reject recompute_granularity="selective" during TrainerRank construction with TrainerRankRuntimeSupportError and an actionable hint to restore full/uniform/one-layer recompute.

This is the first step for #913. Draft follow-up #915 models retained activations and removes the temporary selective-recompute refusal; it is stacked on this PR and awaits native GPU calibration. The provider and other Megatron entry points keep their existing behavior.

Validation:

  • All 276 tests across topology, TrainerRank validation, and split admission passed (269 initially; seven multiprocessing cases passed after correcting the stdin-based test launcher).
  • The selective-recompute rejection tests pass at TP 1/2/4 and all three fail against the original constructor. Full/disabled recompute acceptance is covered as well.
  • Changed-file Ruff lint, formatting, type checks, and uv lock --check pass.
  • Repository-wide type checking reports 23 existing diagnostics in untouched files.

@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 merged commit 7ef066b into main Sep 16, 2026
7 checks passed
@bradhilton

Copy link
Copy Markdown
Collaborator Author

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

Change. Rejects recompute_granularity="selective" at TrainerRank construction with TrainerRankRuntimeSupportError and a hint to restore full/uniform/one-layer recompute: the static activation-memory admission estimate is calibrated for full recompute, so selective runtimes passed admission and OOMed on the first forward (#913). Other Megatron entry points are unchanged.

Review. Minsky and McCarthy posted source-correctness CLEAR at 8aa6d4f; Shannon posted LGTM. Verified: topology tests pass (14/14, new selective-refusal cells at TP 1/2/4), the base fails exactly the three selective cases as a negative control, 72 further constructor controls pass, Ruff/format/ty clean. Hosted quality checks and the 2x H200 job were pending at review time.

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