Skip to content

Exclude inactive requests from TrainerRank memory profile ratios - #880

Merged
bradhilton merged 1 commit into
mainfrom
schulman/art848-active-memory-20260910
Sep 10, 2026
Merged

Exclude inactive requests from TrainerRank memory profile ratios#880
bradhilton merged 1 commit into
mainfrom
schulman/art848-active-memory-20260910

Conversation

@bradhilton

@bradhilton bradhilton commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Inactive forward requests have no requested outputs and execute no model work, but their token lengths currently affect memory-profile scaling. Adding an 8,001-token inactive request to unchanged eight-token work can inflate its estimate from 11,000 to 9,663,622 bytes and cause unnecessary splitting. Including inactive rows during calibration can also discount later active work incorrectly.

Count only executed requests when learning and applying profile ratios and pricing split candidates. Preserve total logical-token telemetry, returned outputs, and all-active admission behavior. Runtime changes are entirely in art.trainer_rank (24 added / 7 removed lines); no art.megatron, TrainerRank API, Caladan or experiment changes are required.

Validation: the original 13 focused tests produce 10 failures and 3 passes on the old source and all pass with this correction. Independent review passes 130 supplied plus 14 additional CPU cases; 64 all-active snapshots are byte-identical to baseline. After applying the identical patch to merged main 077abf6, 113 selected active-memory/split/MoE CPU tests pass. These use controlled memory observations, not measured GPU peaks. Both formal reviews are CLEAR. Hosted two-H200 validation passes 61 tests (one four-GPU case skipped), plus CP2/TP2 smokes with exact head/slot backward parity; full Prek is green. Exact pod/service and supervising process cleanup were independently verified.

Related to #848. This does not resolve mixed-gradient signature reuse, CUDA library headroom, or the historical probe OOM. Held for Brad's morning merge review.

@bradhilton
bradhilton temporarily deployed to trainer-rank-gpu-validation September 10, 2026 02:35 — with GitHub Actions Inactive

@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.

McCarthy: CLEAR at 3192f32ca92ff352f819bb5c128e1d738b59a1ae against 077abf6fa7a3a51f9ff255c37f8886c6c8256082. No merge-blocking findings in this scoped accounting correction.

The active-row predicate matches execution grouping and checkpoint-slot preparation: targets, logits, top-k or hidden states make a request active, counted once regardless of how many outputs it requests. Split lower costs, width estimates, exact plan costs, direct admission and profile calibration now consistently use active logical rows. Total input counts remain in public stats, telemetry and error evidence. Raw estimator/retention rules, signatures, packed/TP-padded geometry, output allocation, current-residency checks and collective ordering are unchanged.

Independent CPU validation at this head:

  • 130 active-memory/split/MoE tests pass. The 13 new active-memory cases on the base produce 10 failures and 3 passes, including the incorrect later-work discount from inactive-heavy calibration.
  • 33 additional controls pass across every requested-output combination and grad mode, all--100 targets (still active decoder work), unloaded inactive checkpoint references, and all-inactive plans with zero cost and no profile update.
  • 64 paired all-active cases have byte-identical cold checks, learned profiles, warm checks, retained costs, split lower costs and packing metadata on base/head, across output types, sharing layouts and TP 1/4.

The revised native retained-boundary test keeps the real 13/15 calibration and 80/104-packed, 960-active-logical witness using active repeated rows; it preserves equality/nextafter and exact-rung coverage without relying on the invalid inactive-row accounting. Ruff, format and diff checks pass.

Evidence: /home/brad/.local/share/mccarthy/art880-review-20260910/. These are controlled CPU observations, not GPU peak measurements or a full #848 resolution. Hosted qualification and Schulman's resource cleanup remain separately owned; merge remains held for Brad.

@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.

Minsky review — CLEAR at 3192f32ca92ff352f819bb5c128e1d738b59a1ae (base 077abf6f). CPU/source scope; the two-H200 CI qualification remains Schulman's gate.

Finding: _active_logical_tokens uses exactly the four conditions (target_tokens, logits, top_k, hidden_states) that _group_active_request_indices uses to decide which rows execute, so priced logical tokens are precisely the rows that enter an execution group. Consumers now split cleanly:

  • Active tokens (pricing/calibration): _plan_cost, _memory_check, _split_chunk_lower_cost (including the post-cap cold bound and the retained-compute trust gate), the cheap width pricing in _estimate_flat_forward's priced, and _update_memory_profile's logical_per_packed. Both sides of the ratio comparisons (logical_tokens / packed vs profile.logical_per_packed) are active-only, so the trust guard stays self-consistent.
  • Total tokens (telemetry/errors): _FlatForwardPlan.logical_tokens, MicroBatchStats, _memory_error contexts, _telemetry_signature, and the plan-set sum are unchanged, matching the stated intent to keep total-input telemetry.
  • Every consumer of active_logical_tokens is typed _FlatForwardPlan, the only class that carries the property; _SplitForwardPlan is only used on the telemetry path. Inactive count is an exact integer subtraction, no float rounding. All-active inputs are numerically unchanged (inactive_logical_tokens == 0).

Verification at this exact head (ART venv):

  • All 13 tests/unit/test_trainer_rank_*.py files: 480 passed in 6m20s. Ruff check and format clean on the three changed files.
  • Negative control: the new test_trainer_rank_active_memory.py against the 077abf6f source → 10 failed / 3 passed, so the suite bites.
  • Patch identity: git patch-id of 077abf6f..3192f32c equals that of b34113b9..b102d6e7 (the privately reviewed commit), and _impl.py hashes to 3767a8c0… as stated in the PR.

P3 (not blocking): the activity predicate is now written out three times (_active_logical_tokens, _group_active_request_indices, and the check near line 4345). A single _is_active(request) helper would prevent future drift between pricing and execution eligibility.

Draft; no merge requested. McCarthy has cleared this same SHA, so both dedicated-reviewer pass-offs are on 3192f32c.

@bradhilton
bradhilton marked this pull request as ready for review September 10, 2026 18:09
@bradhilton
bradhilton merged commit 7b286a6 into main Sep 10, 2026
7 checks passed
@bradhilton
bradhilton deployed to trainer-rank-gpu-validation September 10, 2026 18:09 — with GitHub Actions Active
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