Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request replaces the pickled dictionary representation of Mooncake multimodal URL data with a typed msgspec.Struct named MooncakeMMUrlItem. This allows native msgpack serialization and deserialization without pickle wrapping, improving performance and type safety. Corresponding updates were made in encode_receiver.py to use attribute access instead of dictionary access, and unit tests were added to verify the round-trip serialization of the new struct. There are no review comments, and I have no additional feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
ShangmingCai
left a comment
There was a problem hiding this comment.
@rainj-me, could you also help check this?
|
I validated the current PR head Local checks: git diff --check HEAD^..HEAD
python3 -m py_compile \
python/sglang/srt/managers/io_struct.py \
python/sglang/srt/disaggregation/encode_receiver.py \
test/registered/unit/managers/test_io_struct_msgpack.py
PYTHONPATH=python python3 -m pytest \
test/registered/unit/managers/test_io_struct_msgpack.py -qResults:
I did not run GPU/server validation because this PR only changes the typed IPC |
| ``modality`` tags which encoder pool handles it. | ||
| """ | ||
|
|
||
| url: Any |
There was a problem hiding this comment.
Please treat Any extremely careful since the decode will never deserialize it.
Motivation
Follow-up to #28688, which moved IPC dataclasses to
msgspec.Structand added the opt-in msgpack transport (SGLANG_USE_PICKLE_IPC=0). Several fields still fall back toPickleWrapper. This handles Task 6 of #29465:mm_data_mooncakeonTokenizedGenerateReqInputwas typedOptional[PickleWrapper]and carried a pickledList[{"url": ..., "modality": Modality}]produced byMMReceiverBase._extract_url_data. Giving it a concrete msgspec type removes the whole-field pickle round-trip on the mooncake encode path. It is the most self-contained of the open tasks: one field, a known shape, and a single producer.Refs #29465
Modifications
MooncakeMMUrlItemmsgspec.Struct(url,modality) inio_struct.pyand retypedmm_data_mooncakefromOptional[PickleWrapper]toOptional[List[MooncakeMMUrlItem]].urlis typedAnybecause msgspec cannot express a union mixingstrandbytes; every payload_extract_url_dataproduces (URL string, raw dict, or bytes) is a native msgpack type, so the field round-trips without a wrapper.mm_data_mooncakefromwrap_pickle_fields()/unwrap_pickle_fields()onTokenizedGenerateReqInput.mm_inputsandtime_statsare untouched.MMReceiverBase._extract_url_datato buildMooncakeMMUrlIteminstead of a dict, and switched every consumer inencode_receiver.py(mooncake RDMA dispatch, HTTP encode dispatch, gRPC encode, and_assign_items_by_modality) from["url"]/["modality"]dict access to attribute access.test/registered/unit/managers/test_io_struct_msgpack.pycovering msgpack round-trip ofmm_data_mooncake: populated string URLs, a raw dict payload,None, empty list, and eachModalityvariant.tokenizer_manager.py(passes the field through) andrequest_logger.py(lists it as a log-skip name) need no change.Accuracy Tests
Not applicable: this is a transport type change for an internal IPC field and does not affect model outputs.
Speed Tests and Profiling
Not applicable to accuracy; the change removes a pickle encode/decode for this field on the mooncake path under msgpack IPC.
Checklist
Review and Merge Process
/tag-and-rerun-ci,/tag-run-ci-label,/rerun-failed-ciCI States
Latest PR Test (Base): ❌ Run #28315034919
Latest PR Test (Extra): ❌ Run #28315034857