Repository navigation
chore: AUT-2539 nightly sync main into dev (2026-10-13) - #8209
Conversation
Signed-off-by: mkhona <mkhona@nvidia.com> Signed-off-by: Deepak Narayanan <dnarayanan@nvidia.com> Signed-off-by: Keshav Santhanam <ksanthanam@nvidia.com> Co-authored-by: Deepak Narayanan <dnarayanan@nvidia.com> Co-authored-by: Keshav Santhanam <ksanthanam@nvidia.com>
…ad path (NVIDIA#5551) Signed-off-by: asolergi-nv <asolergibert@nvidia.com>
…VIDIA#6446) Signed-off-by: Jiangfei Duan <jiangfeid@nvidia.com>
…er size (NVIDIA#6379) Signed-off-by: Deepak Narayanan <dnarayanan@nvidia.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…IDIA#5590) Signed-off-by: Deepak Narayanan <dnarayanan@nvidia.com>
NVIDIA#6469) Signed-off-by: Ajay Balasa <abalasa@nvidia.com>
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
Signed-off-by: Jiangfei Duan <jiangfeid@nvidia.com>
…6443) Signed-off-by: Deepak Narayanan <dnarayanan@nvidia.com> Signed-off-by: Keshav Santhanam <ksanthanam@nvidia.com> Signed-off-by: Mikail Khona <mkhona@cw-dfw-cs-001-vscode-01.cm.cluster> Signed-off-by: Mikail Khona (NVIDIA) <mkhona@nvidia.com> Signed-off-by: Kezhi Kong <kezhik@nvidia.com> Co-authored-by: Kezhi Kong <devnkong@gmail.com> Co-authored-by: Deepak Narayanan <dnarayanan@nvidia.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Mikail Khona (NVIDIA) <mkhona@nvidia.com> Co-authored-by: Mikail Khona <mkhona@cw-dfw-cs-001-vscode-01.cm.cluster> Co-authored-by: root <root@nvl72018-T18.cm.cluster> Co-authored-by: Roger Waleffe <rwaleffe@nvidia.com> Co-authored-by: Kezhi Kong <kezhik@nvidia.com>
Signed-off-by: Jingyue Wu <jingyuew@nvidia.com>
Signed-off-by: Philip Petrakian <ppetrakian@nvidia.com>
Signed-off-by: Maanu Grover <maanug@nvidia.com>
Signed-off-by: Deepak Narayanan <dnarayanan@nvidia.com> Signed-off-by: Keshav Santhanam <ksanthanam@nvidia.com> Signed-off-by: Mikail Khona <mkhona@cw-dfw-cs-001-vscode-01.cm.cluster> Signed-off-by: Mikail Khona (NVIDIA) <mkhona@nvidia.com> Signed-off-by: Kezhi Kong <kezhik@nvidia.com> Co-authored-by: Kezhi Kong <devnkong@gmail.com> Co-authored-by: Deepak Narayanan <dnarayanan@nvidia.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Mikail Khona (NVIDIA) <mkhona@nvidia.com> Co-authored-by: Mikail Khona <mkhona@cw-dfw-cs-001-vscode-01.cm.cluster> Co-authored-by: root <root@nvl72018-T18.cm.cluster> Co-authored-by: Roger Waleffe <rwaleffe@nvidia.com> Co-authored-by: Kezhi Kong <kezhik@nvidia.com>
Signed-off-by: Shiqing Fan <shiqingf@nvidia.com>
Signed-off-by: Jingyue Wu <jingyuew@nvidia.com> Co-authored-by: svcnvidia-nemo-ci <svcnvidia-nemo-ci@nvidia.com>
Signed-off-by: Deyu Fu <deyuf@nvidia.com>
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com> Co-authored-by: Jorge Albericio <jalbericiola@nvidia.com>
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com> Co-authored-by: Jorge Albericio <jalbericiola@nvidia.com>
Signed-off-by: Skand Hurkat <shurkat@nvidia.com> Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…etch reads them (NVIDIA#6388) Signed-off-by: Shiqing Fan <shiqingf@nvidia.com> Co-authored-by: Jiangfei Duan <jiangfeid@nvidia.com>
Signed-off-by: Jingyue Wu <jingyuew@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Chaitra Sathe <csathe@nvidia.com>
Signed-off-by: Philip Petrakian <ppetrakian@nvidia.com>
Signed-off-by: Lawrence McAfee <lmcafee@nvidia.com>
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
Signed-off-by: Philip Petrakian <ppetrakian@nvidia.com>
Signed-off-by: Jingyue Wu <jingyuew@nvidia.com>
Signed-off-by: Keshav Santhanam <ksanthanam@nvidia.com> Co-authored-by: Shanmugam Ramasamy <shanmugamr@nvidia.com>
|
/ok to test 2e5f599 |
|
/review reset=true mode=strict |
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
Strict review — incomplete
Findings: CRITICAL: 11, IMPORTANT: 1, SUGGESTION: 0. Overall risk: significant. Eleven concrete findings from review 5529654466 remain unresolved. One additional checkpoint-cache collision is source-validated below. The inline comments contain F1–F5; the remaining findings follow here.
Verified fix: prior F9's HTTP-server signature mismatch is resolved. The real server accepts sock, and the new tests bind both serving APIs to that signature and exercise pre-bound sockets. That finding is closed.
Why the remaining tasks cannot be completed here
The all-changed-file inspection and whole-scope cross-component validation tasks remain pending. This broad review covers 343 changed paths and a 1,879,821-byte diff, including 290 omitted diff sections requiring head/base reconstruction. Substantial kernel, test, and configuration coverage remains, alongside full-context and added-identifier runtime-use audits of partially inspected large files. Completing those obligations with the available read-only operations exceeds this final bounded continuation. The cross-component task also depends on that unfinished inspection. I have therefore left both tasks pending rather than falsely marking them complete.
The twelve published findings are individually source-validated; this is not a clean assessment of the unreviewed changes. This RESET review compares the current head with the supplied dev base: importing code unchanged from main does not eliminate a regression relative to that base. The required protected-base style-guide file was unavailable. No tests were executed; acceptance checks below are proposed. Suggested code was traced against inspected callers and preservation cases but requires author verification.
F6 — Preserve positional model-constructor compatibility
[IMPORTANT Compatibility] Failure: logit_dtype was inserted before parallel_output in GPTModel and HybridModel. For example, GPTModel(config, spec, 128, 32, True, True, False, False) previously selected parallel_output=False; it now binds logit_dtype=False and leaves parallel output enabled. Every later positional argument also shifts. Compare the base GPT signature and base Hybrid signature. This is unresolved prior F6.
Correction: move the new optional parameter after every pre-existing parameter in both constructors. Apply these signature edits independently:
- logit_dtype: Optional[torch.dtype] = None,
parallel_output: bool = True,
@@
vp_stage: Optional[int] = None,
+ logit_dtype: Optional[torch.dtype] = None,Preservation and acceptance — proposed: signature-binding tests must retain every old positional index, including pg_collection and vp_stage. The GPT example must bind parallel_output=False, logit_dtype=None; test the analogous Hybrid positional sequence. Keyword logit_dtype=torch.float32 must still reach the output projection. Retain the GPT builder assertions and Hybrid builder assertions: FP32 logits, parallel_output is False, enabled embedding sharing, and unchanged process-group/virtual-stage forwarding.
F7 — Normalize string handoff prompts before hashing or tensorization
[CRITICAL Implementation] Failure: both client handoff APIs accept strings, but handoff admission tensorizes the prompt directly. With otherwise valid remote metadata, prompt="hello" raises at torch.tensor(prompt, dtype=torch.int64) instead of entering decode. The scheduler calls this without converting the exception into a terminal client response. This is unresolved prior F7.
Correction: change the handoff method's prompt annotation to str | list[int], and replace the tensorization statement with:
if isinstance(prompt, str):
prompt = self.controller.tokenize_prompt(
self.controller.tokenizer, prompt, sampling_params.add_BOS
)
prompt_tensor = torch.tensor(prompt, dtype=torch.int64)This follows the inspected ordinary tokenizer/BOS contract. The normalized list must supply lengths, hashes, deferred transfer state, and final request construction, without applying BOS again.
Preservation and acceptance — proposed: use a tokenizer producing [1,2,3,4] and valid matching handoff metadata. String and token-list submissions must store/hash identical IDs and complete through streaming and non-streaming clients. For strings, add_BOS=True must produce exactly one leading BOS, while false retains ordinary leading-BOS normalization. Pretokenized lists must remain unchanged even when add_BOS is set. Assert that the engine remains alive after submission.
F8 — Exclude imported decode prompts from the prefill-token limit
[CRITICAL Implementation] Failure: import finalization calls ordinary admission, whose prefill guard rejects prompts exceeding context.max_tokens without chunking. A completed 12-token import into a decode context with max_tokens=8, maximum sequence length 32, sufficient KV capacity, one resume token, and two requested generated tokens is therefore FAILED with TokenOverflowError. Expected work is a one-token decode query, not execution of the imported prompt. This is unresolved prior F8.
Correction: explicitly distinguish prefilled imports and bypass only the prefill-token guard. Suggested edits to engine admission and the linked finalizer:
@@ DynamicInferenceEngine._add_request
- self, request: DynamicInferenceRequest
+ self, request: DynamicInferenceRequest, *, prompt_is_prefilled: bool = False
@@
- if len(request.prompt_tokens) > self.context.max_tokens and not self.enable_chunked_prefill:
+ if (
+ not prompt_is_prefilled
+ and len(request.prompt_tokens) > self.context.max_tokens
+ and not self.enable_chunked_prefill
+ ):
@@ DynamicInferenceEngine.add_request: after precomputed_block_hashes
+ *,
+ prompt_is_prefilled: bool = False,
@@
- return self._add_request(request)
+ return self._add_request(request, prompt_is_prefilled=prompt_is_prefilled)
@@ InferenceStateHandoffMixin._finalize_kv_handoff_import: self.add_request(...)
+ prompt_is_prefilled=True,Update handoff test doubles to accept the optional keyword. Do not bypass maximum-sequence-length, generation-budget, or total-KV-capacity checks.
Preservation and acceptance — proposed: exercise real shared validation, not only a mocked add_request. The imported example must enter decode at KV offset 12, query length 1, with zero prefill requests. An ordinary 12-token prompt must still fail without chunking and remain eligible with chunking. Oversized imported sequences/KV footprints must still fail, and generation-length clamping must remain intact. Retain the exact SSM/MTP admission assertions: positions [3,4,5], block IDs [10,11,11], and SSM slot 10.
F10 — Include GTP in MIMO singleton optimizer writer election
[CRITICAL Correctness] Failure: the newly enabled heterogeneous layout separates GTP from DP, but MIMO's singleton replica ID includes only TP/PP/DP. In the TP2/GTP2/DP1/PP1 language layout, both TP-rank-zero GTP peers return (0,0,0). Consequently, singleton optimizer metadata extraction emits duplicate main replicas for the same param_state_sharding_type key, and for extracted parameter groups/scaler when present. Object integrity validation rejects them. This is unresolved prior F10.
Correction: replace _get_replica_id's final return with the following and update its docstring:
replica_id = (
pg_collection.tp.rank(),
pg_collection.pp.rank(),
pg_collection.dp.rank(),
)
gtp_group = getattr(pg_collection, 'gtp_remat', None)
if gtp_group is not None and gtp_group.size() > 1:
replica_id += (gtp_group.rank(),)
return replica_idThe invariant is one main writer per module-wide singleton. Do not apply singleton election to genuinely sharded optimizer tensors.
Preservation and acceptance — proposed: gather singleton metadata across the four language ranks and assert exactly one main replica per module/suffix/key; save/load optimizer state with integrity validation enabled. Absent GTP and size-one GTP must retain the old tuple and TP/PP/DP election. Preserve independent module/chained-optimizer keys, current parameter-ID restoration, and the existing TP-deduplication save assertion. This remains distinct from F11 and F12.
F11 — Retain GTP-inclusive metadata for replicated MIMO model state
[CRITICAL Correctness] Failure: MimoModel.sharded_state_dict overwrites checkpoint metadata with pg.dp_cp, which excludes GTP peers in the new topology. Corresponding replicated Mamba parameters on both GTP peers therefore receive the same main replica ID. This affects direct Mamba parameters, and the gathered in_proj checkpoint tensor. Access-integrity validation sees duplicate main writers. This is unresolved prior F11 and remains independently reproducible on replicated entries after F12's earlier assertion is repaired.
Correction: replace the linked MIMO metadata assignment with:
mod_metadata['dp_cp_group'] = (
getattr(pg, 'dp_cp_gtp_remat', None) or pg.dp_cp
)Keep the GTP-excluded true-replica group separately available for genuinely GTP-sharded weights, as required by F12. Do not redefine pg.dp_cp, since reductions still need its excluded domain.
Preservation and acceptance — proposed: through MIMO's own sharded_state_dict, gather metadata for A_log, convolution parameters, final norm, and gathered/split in_proj; each replicated logical shard must have exactly one main writer. Model-only save/load must round-trip under TP2/GTP2/DP1/PP1. Legacy/non-GTP partial collections must retain fallback behavior. Preserve genuine GTP shard offsets/padding and one writer per offset. Retain the Mamba replica-ID assertions and the inclusive-versus-excluded distinction in the immediately following regression.
F12 — Use explicit topology throughout GTP checkpointing
[CRITICAL Correctness] Failure: heterogeneous MIMO deliberately bootstraps without MPU groups, and this PR enables GTP in that path. Nevertheless, GTP checkpoint metadata construction falls back to global DP rank. Its sharded-tensor branch also invokes a helper that unconditionally queries global DP rank for GTP tensors. Checkpointing therefore raises the data-parallel-group-not-initialized assertion despite supplied explicit groups. A minimal isolated trigger is a GTP-enabled embedding checkpointed through its explicit-group path with MPU uninitialized. This is unresolved prior F12.
Correction: thread the module's explicit GTP-excluded true-replica group through the MIMO metadata producer, the ordinary transformer checkpoint wrapper, the optional intra_dp_cp_group path in the GTP helper, and the direct embedding/single-tensor helper. Both GTP helper branches must use supplied topology instead of querying MPU; grouped experts need their expert replica domain, not the dense one. Keep global lookup only as a fallback when explicit topology was not supplied. Exact API plumbing remains an implementation choice, so no speculative blanket patch is proposed.
Preservation: F11's replicated/gathered tensors require the inclusive domain; genuinely GTP-sharded tensors require the excluded domain and distinct offsets. Using the inclusive rank everywhere would leave nonzero GTP shards without main writers. Preserve TP×GTP offsets, alignment padding, native-FP8 dequantization/backlinks, and SSM projection split/merge behavior.
Acceptance — proposed: with MPU globals uninitialized, construct explicit TP/GTP/replica groups and checkpoint a GTP embedding and TE projection. Assert no global-group lookup and exactly one main writer per distinct GTP offset. Save/load a small heterogeneous MIMO model and optimizer; repeat legacy initialized-MPU calls to retain fallback behavior. Retain the embedding writer-election assertions, GDP padding/split regressions, and the heterogeneous test's train/save/resume assertions.
F13 — Key checkpoint distributions by the complete process group
[CRITICAL Correctness] Failure: the new cache filename uses only the minimum global rank, and the process-global memo uses that same filename before gathering. Independently selected load/save group families need not share this identity: the configuration allows DP loading and expert-DP saving with one cache directory, and both callers pass that directory to their respective wrappers.
A minimal four-rank setup uses DP [0,1,2,3], expert-DP [0,2]/[1,3], and four fully replicated tensors of distinct positive sizes whose original main replica is global rank zero. First compute the cached DP load distribution, then compute the expert-DP save distribution from fresh metadata in the same job/config. Both DP and [0,2] resolve to pg_dist_0.pkl. The DP greedy distribution elects local ranks 0, 1, 2, and 3; reusing it in a two-rank group leaves two tensors with no possible writer. The application code marks those replicas non-main, so integrity validation fails, or shards are omitted when that validation is bypassed. Expected behavior is the same election as uncached computation for the actual group. The base implementation gathered each requested group separately.
Correction: identify both disk and memory entries by complete ordered group membership, not its minimum. At the linked helper, add import hashlib and replace its final two statements with:
ranks = torch.distributed.get_process_group_ranks(parallelization_group)
group_key = hashlib.sha256(
','.join(map(str, ranks)).encode('ascii')
).hexdigest()
return os.path.join(cache_path, f'{PG_DIST_CACHE_FILE_PREFIX}_{group_key}.pkl')Both cache creation and memo lookup already use this helper, so this separates the demonstrated identities without adding a collective or creating overlong filenames. Update the min-rank-identity documentation and require existing min-only caches to be regenerated rather than blindly reused. The minimum-rank writer election itself can remain unchanged.
Preservation and acceptance — proposed: add a four-rank sequential DP-load/expert-DP-save regression with the fresh replicated metadata above. Cached distributions must equal their uncached counterparts, filenames must differ for different memberships, and every logical tensor must have exactly one main writer. Exercise a real save/load round-trip with integrity validation. Clear the process memo and repeat READ against caches created separately for both group families. Retain one file writer per group, both ignore_groups variants, at-most-once gathering for repeated identical identities, trivial-group/default-uncached behavior, and the existing READ-must-not-gather assertions and cached round-trip value assertions.
Note: formal approval is not available for this request, so this review is posted as a comment only.
Review generated by codex
| # identical block-to-slot mappings on every stage. A recurrent prefix | ||
| # is executable only when all stages retain its state, so additional | ||
| # slots on an individual stage would not increase usable cache capacity. | ||
| if get_pg_size(self.pipeline_parallel_group) > 1: |
There was a problem hiding this comment.
[CRITICAL Correctness] F1 — Every PP peer must join cache-capacity negotiation
Failure: initialize PP=2 HybridModel pattern M*|**, prefix caching enabled, a positive Mamba-cache budget, and no explicit Mamba-memory ratio. HybridModel selects stage-local layers, and from_model returns no recurrent config on the attention-only stage. The allocation guard consequently calls this PP MIN collective only on the recurrent stage, which waits indefinitely. Both stages have attention, so the attention-free-layout guard does not reject the configuration. Expected initialization completes on every stage. This is unresolved prior F1, comment 4280274952 in review 5529654466.
Correction: separate all-peer capacity negotiation from local recurrent allocation. Every PP stage must join in identical collective order when the cache option is enabled. Nonrecurrent stages must contribute a non-limiting capacity, neither divide by a zero footprint nor allocate recurrent buffers. Pass the negotiated minimum into local allocation. Broadening the current allocator guard alone is unsafe; no one-line patch is suggested.
Preservation: retain the repaired scratch reservation, hard errors when a recurrent stage cannot fit one durable slot, and synchronized minimum capacity when every stage has recurrent layers. PP=1 and disabled caching must retain ordinary behavior.
Acceptance — proposed, not executed: initialize both contexts for M*|** under a bounded distributed-test timeout. Assert completion, no Mamba allocator on the attention-only stage, and budget-respecting recurrent allocation. Retain the uneven-layer test's identical-capacity assertion, local insufficient-budget errors, and the remote-limiting-stage assertions, including the message identifying room for three local durable slots.
| reasoning = metadata.get("reasoning") or "" | ||
| deltas = [] | ||
|
|
||
| if reasoning.startswith(self._reasoning_sent): |
There was a problem hiding this comment.
[CRITICAL Implementation] F2 — Do not commit reasoning that subsequent parsing can retract
Failure: use StreamingChatParser(lambda t: DeepSeekR1ReasoningParser.parse(t)); parse hello</thi, then finalize hello</think>world. The first call commits reasoning hello</thi; final reasoning is hello, which no longer starts with _reasoning_sent, so the fragment cannot be corrected. Expected concatenation is reasoning hello, content world. Opening-tag splits have the same reclassification problem. See the whole-output parser. Content-marker buffering below does not protect this reasoning branch. This is unresolved prior F2, comment 4280274959 in review 5529654466.
Correction: implement parser-aware stable emission, withholding incomplete reasoning markers and configured implicit boundaries before committing deltas. Emitted text must not later change channels or be retracted, and concatenation must equal final whole-output parsing. Also account for Nemotron's empty-content override: stripping marker suffixes alone cannot prevent that channel reclassification. Exact incremental implementation remains a design choice.
Preservation: unconfigured <tool_call> remains literal reasoning; configured markers end reasoning while remaining available to the downstream tool parser. An unfinished literal hello</thi must be flushed at finalization, not discarded. Retain ordinary content and stable tool IDs/names/JSON arguments.
Acceptance — proposed, not executed: test every split and character-at-a-time delivery of opening/closing tags and configured implicit boundaries, including Nemotron's enable_thinking=False and force_nonempty_content=True. Assert final channel concatenation equals whole-output parsing. Retain the literal-marker assertion and Qwen streaming assertions: exact Checking. content, one name/ID emission at index zero, JSON {"city":"SF"}, and tool_calls finish reason.
| "Quarantining request %d cache storage after an incomplete handoff", | ||
| pending.request_id, | ||
| ) | ||
| if not pending.future.done(): |
There was a problem hiding this comment.
[CRITICAL Correctness] F3 — Send terminal replies for failed, unadmitted handoffs
Failure: submit a coordinator-routed handoff, then fail transfer submission or polling. This branch completes only the engine-local future exceptionally and continues. The scheduler discards that future, and no ordinary request record exists before finalization. No ENGINE_REPLY reaches the client: its future/stream hangs while routing state and pending load remain allocated. Expected behavior is one terminal FAILED response without blocking unrelated requests. This is unresolved prior F3, comment 4280274966 in review 5529654466.
Correction: add exactly-once terminal failure reporting for unadmitted handoffs on the MP coordinator, retaining the local exception for direct-future callers. Construct a serializable request/error: event serialization requires ContextErrorFactory support; an arbitrary RuntimeError error-event payload is insufficient. Use a supported error carrying the failure or register a handoff-specific error. The normal reply handler can then release routing state and pending load. Exact record construction is an implementation choice.
Preservation: client completion must not imply destinations are reusable. Keep quarantine for potentially active operations, exactly-once safe KV/SSM release, immediate MP failure notification, and admission only after all ranks finish.
Acceptance — proposed, not executed: extend the submission-failure test through mocked coordinator/client framing. Assert one FAILED reply, client resolution/stream termination, routing-map removal, and one pending-count decrement. Retain its direct RuntimeError and release list [[10,11,12]], plus peer-failure quarantine without release. A subsequent valid request must complete.
| return future | ||
|
|
||
| started = self._try_start_kv_handoff_import(handoff) | ||
| if not started: |
There was a problem hiding this comment.
[CRITICAL Correctness] F4 — Reject impossible imports before FIFO insertion
Failure: with pool_size=4 (three usable blocks), block size four, and otherwise valid limits/metadata, submit a 16-token/four-prompt-block handoff. It cannot fit even in an empty pool, but this branch queues it as temporary backpressure. The FIFO drain stops at that head, indefinitely stalling later fitting handoffs. Ordinary permanent KV-capacity validation runs only after import. Expected behavior is a terminal capacity rejection, not indefinite FIFO occupancy. This is unresolved prior F4, comment 4280274976 in review 5529654466.
Correction: before FIFO insertion, distinguish impossible resident footprints from temporary occupancy. Validate prompt plus required resume-continuation storage against total usable capacity; at minimum a prompt exceeding allocator.pool_size - 1 is permanently impossible. Reject through a serializable terminal capacity response, not an uncaught scheduler exception or unresolved future. No destination allocation/transfer should start for a permanently rejected import; coordinate rejection with the push path so it does not leave unmatched operations. Exact failure-reporting integration remains an implementation choice.
Preservation: fitting requests must still defer during temporary shortages and start when capacity returns, preserving NCCL receive order. Cached-prefix blocks consume resident capacity but are not new allocations; retain their references/protection and suffix-only NIXL pulls. Do not trim NCCL's matched source push as though it were a NIXL pull.
Acceptance — proposed, not executed: submit the four-block example followed by a one-block prompt needing one continuation block. Assert the first gets a terminal error, never enters the FIFO, and posts no transfer; the second starts. Retain the temporary-shortage test's exactly-one pull after capacity returns, the cached-prefix test's suffix-only pull and predecessor hash, and the neighboring per-stage/per-rank NIXL trimming and unchanged NCCL source-push assertions.
| self._record_handoff_completion_notification(int(data[1]), bool(data[2])) | ||
| elif header == Headers.ABORT_REQUEST: | ||
| request_id = int(data[1]) | ||
| entry = self.requests.get(request_id) |
There was a problem hiding this comment.
[CRITICAL Correctness] F5 — Route aborts through deferred and pending handoff state
Failure: submit a streaming handoff while capacity is unavailable or transfer is pending, then close the stream. The client sends ABORT_REQUEST, and the coordinator forwards it. This lookup finds nothing because handoffs enter self.requests only during finalization. Cancellation is silently ignored; the canceled request can later import and run generation. Expected cancellation covers the entire request lifecycle. This is unresolved prior F5, comment 4280274981 in review 5529654466.
Correction: consult deferred/pending handoff state before the active-request lookup. Deferred cancellations must never start later; pending transfers must be marked canceled and prevented from entering decode. Complete the internal future and coordinator routing lifecycle exactly once. Retain destination ownership until transport completion is safe: cancellation does not permit recycling KV/SSM storage, and posted NCCL operations must preserve matched ordering. Exact transport-aware cancellation implementation requires lifecycle changes rather than a one-line lookup patch.
Preservation: retain active-request stop behavior, successful uncanceled handoffs, exactly-once safe release, and quarantine of operations that may still access buffers.
Acceptance — proposed, not executed: test abort before capacity admission, during a nonterminal transfer, and after ordinary admission. Restoring capacity after deferred cancellation must cause no transfer or forward. Completing pending handles must add no decode row and release safe storage exactly once; coordinator routing must clear. Retain the client stream-close assertions, the unsafe-reset no-release assertion, and normal active-request completion.
Sync review dispositions (head 2e5f599)This covers reviews 5516528726 (7b81c45), 5529654466 (a6d4c20) and 5531673257 (2e5f599). Each cited range was traced with None of the 62 cited ranges contains a line from the merge commit 4ade8e7. Lines from the repair commit appear only in the F1 anchor (
On the RESET point: agreed. Compared with the dev base, these behaviours arrive with this sync. The sync contract is to import main's 100 commits unchanged and repair only merge or compatibility breakage. Fixing main-owned code only on dev would make the two branches diverge and conflict on every later slice. Recommended route: land the fixes on main, and they will reach dev in a later slice. If maintainers want a specific finding fixed in this PR instead, name it and it will be added to the single repair commit. DCO: 3 of the 102 commits have no |
FDecaYed
left a comment
There was a problem hiding this comment.
LGTM for this sync. I reviewed the conflict resolutions and the latest socket integration fix, and found no remaining merge-specific blockers. The main-origin issues should be fixed on main and brought over in later syncs.
Nemo_CICD_Test passed. Please resolve the conflict with the latest dev and address the patch-coverage check before merging.
Merge NVIDIA/Megatron-LM main slice tip 19deef6 (oldest 100 commits of dev..main, 897649d..19deef6) into dev 87effd0. Conflicts resolved surgically, preserving dev-only features; CODEOWNERS and the pyproject.toml/uv.lock/Dockerfile.ci.dev triple are identical to dev. Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
Repair commit for the 2026-10-13 main-to-dev sync merge 1eaec47. - inference: restore main NVIDIA#5301 pre-bound socket support (the sock parameter) in start_text_gen_server. The slice's NVIDIA#6513 makes MegatronLLM.serve() and MegatronAsyncLLM.serve() pass sock=, but dev's server lacked NVIDIA#5301, so serve() raised TypeError on the primary rank. Add tests that bind both serve() calls to the real signature and cover the pre-bound socket path. - inference: restore main NVIDIA#5348/NVIDIA#6584 mamba-cache scratch reservation and budget-too-small / PP-limiting-stage errors in DynamicInferenceContext (the dev warn-and-disable path was an artifact of revert NVIDIA#7283), and restore test_mamba_cache_budget_too_small_raises. - megatron_fsdp experimental: keep the symmetric-memory all-gather buffer rendezvous working on the dev torch build, which lacks symm_mem.is_symm_mem_tensor. - functional moe_perf: restore the per-iteration CUDA RNG tracker state reset from NVIDIA#7094. - unit test doubles: align with merged APIs (RecordingCheckpoint retain_input_tensors, paged-stash config cuda_graph fields, profile_ranks in test_training, contextmanager import and the dev pipeline_p2p_fixed_shape config field in test_schedules). - unit test isolation: reset the global CUDA graph record after the dev TestMHCWithCudaGraph local-impl tests and before main NVIDIA#6432's TestTransformerLayerCudaGraphManagers, so its create_cudagraphs() does not capture stale mHC layers recorded under the cudagraphable RNG tracker. - unit test teardown: Utils.destroy_model_parallel resets the TE FP8 global state and the HybridEP buffer that cache process groups, and retires the groups parallel_state created; a new autouse conftest fixture destroys them once the test finishes. They are not destroyed inside destroy_model_parallel because a test may re-initialize and keep using a group it captured earlier (test_upcycling_TE reuses its ProcessGroupCollection). Before this, each test leaked its NCCL communicators. The tests/unit_tests/transformer bucket then crossed vm.max_map_count (65530) at test_vision_cuda_graphs, on both this branch and dev c75a269. Before destroying the retired groups, the fixture clears DTensor's per-thread sharding-propagation caches (Python and native). They key on value-equal DeviceMeshes, so a later test that builds a mesh with the same layout would otherwise get placements on the earlier mesh, whose group is destroyed (test_gpt_hybrid_interop fsdp4-dense). - rl unit tests: a bucket-scoped autouse conftest fixture runs gc.collect() before the retired groups are destroyed. test_get_logprobs_cuda_graphs leaves CUDA graphs that captured those communicators reachable only through reference cycles, and ncclCommDestroy waits for them to be freed, so the teardown hung. Collecting suite-wide instead changed object lifetimes in other buckets, so the collection stays in tests/unit_tests/rl. Signed-off-by: svcnemo-autobot <svcnemo-autobot@nvidia.com>
2e5f599 to
4a534df
Compare
|
/ok to test 4a534df |
TransformerConfig rejects activation_func_clamp_value for SwiGLU when
num_moe_experts is None ("only supported with MoE"; added on dev by
NVIDIA#4481 and kept when NVIDIA#5940 consolidated the SwiGLU clamp checks). The
dense MLP already applies the clamp in its fused (bias_swiglu_impl) and
unfused GLU paths, and the GLM-5.3 dense FFN clamps gate/up like its
experts, so a dense-only clamped SwiGLU configuration cannot be built.
Remove only the MoE-only check. The other SwiGLU clamp checks stay: the
clamp must be finite and positive, glu_linear_offset must be zero, and
use_te_activation_func must be False because the TE activation function
does not clamp.
The TE op-fuser dense MLP (TEFusedMLP, and TEFusedMLPWithGroupedLinear
through it) builds its activation from TE ops that do not clamp. Without
the MoE-only check, a dense-only model with
use_transformer_engine_op_fuser would silently run an unclamped SwiGLU,
so TEFusedMLP now rejects activation_func_clamp_value when it is built,
and the field docstring says so. In-tree MoE models are unaffected:
their dense layers come from specs that never select TEFusedMLP. The
check also covers custom specs that do.
Tests:
- test_swiglu_fusion.py: replace the NVIDIA#5940 case that expected the
MoE-only error with a check that a dense clamped config is accepted.
- test_dense_swiglu_clamp.py: real dense MLP, fused and unfused, with
and without the clamp, against a native reference for output, input
and weight gradients under IEEE FP32 matmul; use_te_activation_func
stays rejected for dense and MoE configs; TEFusedMLP and
TEFusedMLPWithGroupedLinear refuse the clamp.
NVIDIA#5940 (merged into dev by NVIDIA#8209) rewrote the SwiGLU clamp block in
transformer_config.py and added the finite/positive and
glu_linear_offset checks. Only the MoE-only check is removed from that
block.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Robin Zhang <robinz@nvidia.com>
Background
This is the nightly
main→devsync. It merges the oldest 100 commits ofdev..main,897649d0..19deef67.87effd07995c95b21e1a53bcfcc11dabbff8c485main:19deef67f910c96c213f33b33b30277be8b94d6dc0d3b03f([data] Support Nemotron-SFT, RP2, and CodeForge datasets in the varlen loader; prefetch packed global batches #5771).git merge-treemerges it into this head with no conflict, so the branch was not re-merged.What changed
The branch is 102 commits ahead of
dev:maincommits, unchanged and in order (checked against the slice list).1eaec4752693a60f5d8a85baa065e0ab15312c9a, with parents87effd07(dev) and19deef67(main).4a534df611eeb12250fad0b09a1a9d2eccf83fec: 14 files, +363/−38. Its patch is identical to the previous repair. It covers:main's optionalsockargument on the dynamic text-gen server, which the Thread socket argument through MInf API #6513 callers use. A new unit test covers it.Details
87effd07, picking up 6 dev commits: [dev] Restore expert wgrad overlap with shared experts #7390, [dev] Fix a race in in-place fused MLA RoPE with remove_interleaving=False #7789, [dev] Share the fused compact DSA indexer workspace across CSA layers #8104, [dev] Fix fused MLA RoPE key/value split for non-power-of-two head dims #7803, [dev] Use binary search for the THD row lookup in fused MLA RoPE #7806 and [dev] chained optimizer fix for cuda graph #8103.test_random.py. The rebuild added one new conflict, intests/unit_tests/fusions/test_mla_yarn_rope_apply.py. It was resolved as a union that keeps main'stest_inplace_forward_backwardand dev'stest_kv_split_non_power_of_two_head_dims..github/CODEOWNERS,pyproject.toml,uv.lockanddocker/Dockerfile.ci.devare identical todev.tl.debug_barrieris a race fix.dsa_compact_indexer_workspace_sharingdefaults to True.git show --remerge-diff 1eaec475shows 60 files.main. Fixes are in separate PRs againstmain: fix(inference): AUT-2539 keep PP stages consistent when only some hold recurrent #8250, fix(dist-checkpointing): AUT-2539 key the PG distribution cache by the full proce #8251, fix(inference): AUT-2539 hold back unstable reasoning in streamed chat deltas #8254, fix(inference): AUT-2539 reply to a KV handoff import that fails before admission #8255, fix(inference): AUT-2539 handle string prompts and rejections in KV hand #8258 and fix(inference): AUT-2539 fail impossible KV handoffs on arrival and honor aborts #8267.Run MBridge testswas requested through the approved mechanism.Tested
Local H100 runs used image
megatron-lm@sha256:2cbc1243aead76636d4a558e5822bbb5466c4361b324f8396e1a95ec4c1b55ed. The final sweep ran on4a534df6, with rows chosen by what the dev changes affect.tests/unit_tests/**/*.pymodels/**transformer/**transformer/moe/**a2a_overlap/**test_multi_latent_attention.py: 13 skips, all declared in the test filetest_dsv4_hybrid_attention_cp_cuda_graph.py: 3 skips, which need SM100+ or are covered under CP2tp1_pp2_dsv4_hybrid_mhc_mtptp2_pp2_mlatp2_pp2_dsa: smoke run only, because no golden values existcommon/moe_perfand..._logitsmatchare left to the full-suite CI.Ordered 100 source SHAs
897649d 14346b6 15c83d2 81fe7c7 8534490 fad6111 b2f2888 5436289 a34880f 3cd8cf3 3ce4b4a 301e0d6 5714a20 fecb29f 8ba5c60 281075d 2a75ac1 87bf520 487bb39 09921c0 2119719 87cc590 d7430d5 f7b2209 69e282e 3d0956f 0ee415b 94939b8 f9cc544 de4a82e d918967 d6d48cc ad3ecd9 6c34429 d347eef 4dc5c96 c8b2a4a 41ba4d9 cac6b7b 814300c 78ad108 883533e 0615ac9 a4417a5 669d6ed 51f6a39 e79110c e60e42f 9e96f45 c95b0da b164d74 c374e5b d262dbe 34c3741 32e7eda 62a6173 afb55dd 43c0e02 8638022 51270a3 fcc3319 dd8c825 39d0896 e4ba3a3 7cdba42 d7e2c13 574e04e 59ff171 35ae46c 5d50b16 32756e3 ae5ec14 cfb8e46 dcb2bfd bd72caf c9676f4 3126ff2 74a4894 0980313 54c7918 d5a019a aaa4dfd aeae5d7 5f9b2bb 342faa4 e5542e8 bf51cf4 0961f12 e7d58db 29361d6 c452d5a 3ad8c2a 540f420 1d69364 afa707e cab7116