MInf: Add hooks to preprocess and stage - #7015
Conversation
|
This PR has been automatically converted to draft because all PRs must start as drafts. When you are ready for review, click Ready for Review to begin the review process. This will:
See the contribution guide for more details. |
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
e7b959b to
d64c7ad
Compare
|
/claude strict-review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Strict Review Summary — PR #7015 "MInf: Move metadata out of RESTful wires" Findings: CRITICAL: 5 · IMPORTANT: 9 · SUGGESTION: 2 (16 inline comments) Most impactful
Other themes
Risk assessment: High One deterministic CI failure plus several silent-failure paths, all on the configuration this PR makes the MRL default ( |
C12: move offload_params out of the metadata frame into a fifth client frame that the coordinator forwards verbatim (never decoded), so the frame it unpacks and repacks per request stays bounded; the engine reads it from its own frame at admission and the preparer rewrites only the prompt and offload frames. C11: schedule_requests logs a warning and drops a SUBMIT_REQUEST with the wrong frame or metadata-field count instead of raising, so one version-skewed client cannot take every MP rank down; the skip is collective because all ranks see the same broadcast list. C13: _prepare_submit_request_message returns the message untouched, without decoding any frame, when the offload frame is msgpack nil, since the preparer only has work when the client sent params. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Laura Dang <laurad@nvidia.com>
…contract - serialize(): an offloaded reply never carries the prompt tensors (the stager holds the prompt ids); otherwise they stay opt-in via return_prompt_tokens and are dropped when sampling_params is None, as on main. - _send_requests_to_coordinator(): build FinishedRequestRecord once per completed request and share it between the ledger and _serialize_finished_request(). - OffloadedRequestPayload.from_request(): document the prompt_tokens host-copy side effect. - payload_offloaded / payload_stage_metadata: reword the field comment; they are wire-only keys kept for deserialize(). - checkpoint()/merge(): share offload_params instead of deep-copying; nothing mutates it in place. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Laura Dang <laurad@nvidia.com>
The finished-request rename left two references to the old merged record in the GPU-only offload test. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Laura Dang <laurad@nvidia.com>
… in offload_params E19: /v1/completions now collects each reply's payload_stage_metadata, rejects conflicting values across a batch, and merges it into the top-level response body with the same reserved-field check as /v1/chat/completions; the shared logic lives in endpoints/common.py (collect_stage_metadata, attach_stage_metadata). The endpoint also accepts and forwards offload_params, which it previously ignored. E20: both endpoints reject client-supplied offload_params whose top-level keys start with '_' (HTTP 400) via endpoints.common.validate_offload_params, so the engine-owned _request_prompt_preparation_error field cannot be forged from a request body. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Laura Dang <laurad@nvidia.com>
Signed-off-by: Laura Dang <laurad@nvidia.com>
|
Tested TQ + Router replay in nemo-rl, using this branch. Seems to work. |
Resolve conflicts with NVIDIA#7352 (raw-tensor serialization and vision caching): - dynamic_engine.py: keep both offload_params and media_cache_key on add_request/_build_vlm_request; unpack the offload frame inside the nvtx-wrapped SUBMIT_REQUEST decode. - chat_completions.py: pass prepared_multimodal_data together with offload_params at both submission sites. - test_inference_request.py: compare unwrapped tensor values while keeping the payload_offloaded case. - test_chat_completions.py: let the fake client accept offload_params. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Laura Dang <laurad@nvidia.com>
|
/ok to test dd5439b |
Two tests in test_dynamic_engine.py were left inconsistent by the merge of
main into this branch:
- test_schedule_requests_skips_cached_media_payload_and_preprocessing came
from main with a three-frame SUBMIT_REQUEST. This branch moves
offload_params into a fourth frame, so the engine dropped the message as
malformed and add_request was never called. Send the packed-None offload
frame and expect offload_params=None in the add_request kwargs.
- test_payload_offload_stages_only_eligible_completed_replies compared the
wire prompt tensors against the legacy ("tensor", [1, 2, 3]) list form.
Main now serializes tensors as binary dicts, so compare the unwrapped
values via unwrap_serialized_tensors instead.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Laura Dang <laurad@nvidia.com>
|
/ok to test 6dc0789 |
Resolve the conflict in chat_completions.py against NVIDIA#7312: keep this branch's restructured prefix-replacement block and fold in main's fallback to prompt_token_ids when compact_prompt_token_ids are absent and no media is present. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Laura Dang <laurad@nvidia.com>
|
🔄 Merge queue validation started! You can track the progress here: https://github.com/NVIDIA/Megatron-LM/actions/runs/35426178916 |
This branch was successfully deployed
7 participants
What does this PR do?
Issue tracking
For PRs from open-source community contributors:
Linked issue:
Contribution process
Pre-checks
Code review
Feel free to message or comment @NVIDIA/mcore-oncall to help accelerate your merge into main. The less complex your PR is, the faster it will be approved and merged!
All PRs start as draft. If you open a non-draft PR, it will be automatically converted to draft.
Step 1: Mark PR as "Ready for Review"
.github/CODEOWNERS.Final Review might get declined if these requirements are not fulfilled.
Step 2: Final Review
For PRs that change
megatron/core, once all expert reviewers have approved, theFinal Reviewlabel is applied automatically and final reviewers are assigned.For PRs outside
megatron/core, this step is skipped.Step 3: Approved
Once all required reviewers have approved, the
Approvedlabel is applied automatically.Merge
Any member of mcore-engineers will be able to merge your PR.