Repository navigation
Conversation
GetModelInfo over gRPC always reports `supports_vision: false`
(and falls back to default vocab_size / eos_token_ids / pad /
bos / is_generation) for every model. The smg-grpc-servicer
SGLang bridge reads these fields out of `scheduler_info` via
`.get(..., default)` (see smg_grpc_servicer/sglang/server.py
and servicer.py), but `Scheduler.get_init_info()` only ever
returned `{status, max_total_num_tokens, max_req_input_len}`,
so every other key silently falls back to its hardcoded
default. For multimodal models like Nemotron-3-Nano-Omni this
makes the worker invisible to image/audio routing through the
gateway. This patch populates `is_generation`, `supports_vision`,
`vocab_size`, `eos_token_ids`, `pad_token_id`, and `bos_token_id`
from `self.model_config` and `self.is_generation`, mirroring
what the vLLM servicer reads from `model_config.is_multimodal_model`
at smg_grpc_servicer/vllm/servicer.py:273. A second hunk fixes
`DataParallelController`, which rebuilds the upstream handshake
dict from scratch at line 651 and would otherwise strip the new
fields when `dp_size > 1`; the controller now caches the first
child scheduler's full init dict and spreads it through the
pipe send. Verified on a live `--grpc-mode` Spark worker serving
nvidia/Nemotron-3-Nano-Omni-30B-A3B-Reasoning-FP8: GetModelInfo
now correctly returns supports_vision=True, vocab_size=131072,
and eos_token_ids=[2, 11] where they previously came back as
False / 128256 / []. HTTP-mode consumers are unaffected.
There was a problem hiding this comment.
Code Review
This pull request updates the scheduler initialization information to include essential model metadata such as vision support, vocabulary size, and token IDs, ensuring these fields are correctly propagated through the data parallel controller. The review identified a potential crash risk in the handling of eos_token_ids due to incorrect type handling and falsy value evaluation, which requires a more robust implementation to handle both integer and list types correctly.
| hf_cfg = self.model_config.hf_config | ||
| pad_id = getattr(hf_cfg, "pad_token_id", None) | ||
| bos_id = getattr(hf_cfg, "bos_token_id", None) | ||
| result_dict = { | ||
| "status": "ready", | ||
| "max_total_num_tokens": self.max_total_num_tokens, | ||
| "max_req_input_len": self.max_req_input_len, | ||
| "is_generation": self.is_generation, | ||
| "supports_vision": self.model_config.is_multimodal, | ||
| "vocab_size": self.model_config.vocab_size, | ||
| "eos_token_ids": sorted(self.model_config.hf_eos_token_id or []), | ||
| # int32 proto field; coerce missing IDs to the smg-grpc-servicer | ||
| # int defaults (0 for pad, 1 for bos) so encoding doesn't crash | ||
| # on `None`. | ||
| "pad_token_id": pad_id if pad_id is not None else 0, | ||
| "bos_token_id": bos_id if bos_id is not None else 1, | ||
| } |
There was a problem hiding this comment.
The logic for eos_token_ids has two issues:
- If
hf_eos_token_idis a single integer (common in many models),sorted()will raise aTypeErroras integers are not iterable. - The use of
or []will treat a token ID of0as falsy and return an empty list instead of[0].
It's safer to explicitly handle the integer case and check for None.
References
- Defensive programming: ensure appropriate handling of different types (int vs list) and edge cases (0 as a valid ID).
There was a problem hiding this comment.
Thanks for the careful read. Both concerns are technically defused by an upstream invariant, but you're right that the diff doesn't show that — so I've pushed a defensive rewrite in 83c2f89 that makes the safety locally obvious.
Details on the original code, for the record:
-
Bare
int→TypeError—model_config.hf_eos_token_idis populated byModelConfig._get_hf_eos_token_id, which normalizes HF'sint | list[int] | Noneinto aSet[int](line 1307 wraps a bare int with{eos_ids}, line 1309 substitutesset()forNone). By the timesorted()sees it, it's always a set. -
or []drops0—orwas operating on the set, not a scalar.bool({0}) is True, so{0} or [] == {0}andsorted({0}) == [0]. The falsy-zero footgun is real for scalarintfields but not for sets.
That said, the producer's type annotation is Optional[Set[int]], which advertises a None return the function never actually produces. The handshake shouldn't depend on a cross-file invariant the type system doesn't enforce, so the new commit coerces inline:
eos_ids = self.model_config.hf_eos_token_id
eos_token_ids = sorted(
[eos_ids] if isinstance(eos_ids, int) else eos_ids or []
)isinstance(int) runs first so token ID 0 survives (isinstance(0, int) is True, [0] or [] == [0]). Same behavior today, robust against future refactors.
sorted(hf_eos_token_id or []) silently relied on ModelConfig._get_hf_eos_token_id normalizing int|list|None into a Set[int] before this site runs. That contract is invisible from this hunk — the producer's annotation is Optional[Set[int]] — so a future refactor could legitimately break the handshake. Coerce inline (wrap a bare int, treat None as empty) so the serialization is robust without depending on a cross-file invariant that the type system doesn't enforce.
|
Thanks @laudney. Closing this because it has had no updates in 112 days. Reopen it if the work is still relevant. Some directories moved recently, so an older branch may need retargeting: |
Summary
Scheduler.get_init_info()now emitsis_generation,supports_vision,vocab_size,eos_token_ids,pad_token_id, andbos_token_idalongside the existing three keys, sourced fromself.model_configandself.is_generation.DataParallelController.run_data_parallel_controller_processnow spreads the first child scheduler's full init dict into the upstream pipe send instead of rebuilding it from scratch, so DP-mode workers don't strip the new fields.Why
GetModelInfoover--grpc-modewas reportingsupports_vision: falsefor every model — including genuinely multimodal models likenvidia/Nemotron-3-Nano-Omni-30B-A3B-Reasoning-FP8(architectureNemotronH_Nano_Omni_Reasoning_V3, which is registered inmultimodal_model_archsatpython/sglang/srt/configs/model_config.py:1518).Root cause is a long-standing dialect mismatch between SGLang's scheduler and the smg-grpc-servicer it talks to:
smg_grpc_servicer/sglang/server.py:81-97builds themodel_infodict thatGetModelInforeturns by readingscheduler_info.get("supports_vision", False),.get("vocab_size", 128256),.get("eos_token_ids", []),.get("pad_token_id", 0),.get("bos_token_id", 1).smg_grpc_servicer/sglang/servicer.py:325-329also readsscheduler_info.get("is_generation").Scheduler.get_init_info()only ever returned{status, max_total_num_tokens, max_req_input_len}. Every other key was absent, so all six reads silently fell back to their hardcoded defaults.The bug has been present since #10283 (the original gRPC server PR) and was carried forward through #20478 (the standalone-package extraction). It went unnoticed because most
--grpc-modedeployments to date have been text-only LLMs wheresupports_vision: falsehappened to be the correct answer.The vLLM servicer counterpart at
smg_grpc_servicer/vllm/servicer.py:268-275reads these same fields directly frommodel_config.is_multimodal_model, etc.; the SGLang bridge was written expecting them viascheduler_info, but SGLang's scheduler never put them there.What the patch does
Scheduler.get_init_info()(scheduler.py)Adds six new keys to the dict it sends up the IPC pipe, sourced from objects that already exist on the scheduler at the time the pipe send runs:
is_generationself.is_generationinit_tokenizer()before scheduler is exposedsupports_visionself.model_config.is_multimodalis_multimodal_modelvocab_sizeself.model_config.vocab_sizeeos_token_idssorted(self.model_config.hf_eos_token_id or [])sorted()for deterministic wire order; set→list conversion needed for protorepeated int32pad_token_idgetattr(hf_config, "pad_token_id", None)(coerced)None-coerced to 0 (protoint32can't carry None)bos_token_idgetattr(hf_config, "bos_token_id", None)(coerced)None-coerced to 1The
None-coercion usesx if x is not None else defaultrather thanx or default, which correctly passes through a legitimatepad_token_id = 0(common in many models).DataParallelController(data_parallel_controller.py)When
dp_size > 1, the SGLang gRPC launcher spawnsrun_data_parallel_controller_processinstead of per-rank schedulers. The controller previously rebuilt the upstream handshake dict from scratch with just{status, max_total_num_tokens, max_req_input_len, SCHEDULER_PIDS_ARG}, which would silently strip every field this PR adds toget_init_info(). The fix:self.scheduler_init_info.run_data_parallel_controller_processwith{**controller.scheduler_init_info, SCHEDULER_PIDS_ARG: scheduler_pids}.The existing
self.max_total_num_tokens/self.max_req_input_lenattributes are kept in place becausepython/sglang/srt/ray/engine.py:285-286reads them.Test plan
Verified live against
sglang serve --grpc-modeon a DGX Spark worker servingnvidia/Nemotron-3-Nano-Omni-30B-A3B-Reasoning-FP8.GetModelInfobefore vs after:supports_visionfalsetruevocab_size128256131072eos_token_ids[][2, 11]is_generationtruepad_token_id0(default)0(real, coincidence)bos_token_id1(default)1(real, coincidence)Pre-commit (
isort/ruff/black/codespell) clean on the two changed files.Patch applies cleanly against
main(5227b0766) and the v0.5.11 release tag.No unit tests added —
get_init_inforeturns a snapshot of objects that themselves have coverage elsewhere; the meaningful end-to-end signal is the gRPCGetModelInforound-trip, which lives outside this repo in the smg-grpc-servicer package. Happy to add a unit test intest/registered/unit/managers/test_scheduler_init_info.pyif reviewers prefer.HTTP mode regression — the
tokenizer_manageronly readsmax_req_input_lenfrom the init dict (http_server.py:222), so the additional keys are no-ops for HTTP consumers.Related