Repository navigation
fix(sglang): stop an unusable mooncake backend crashing workers after model load - #14953
Conversation
… model load (#14461) Signed-off-by: svc-glamr@nvidia.com <svc-glamr@nvidia.com> Signed-off-by: glamr-agent <glamr-agent@users.noreply.github.com> Signed-off-by: GLAMR <svc-glamr@nvidia.com> Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
| # carries neither flag, so both would leave this unchecked. parsed_args | ||
| # comes from ServerArgs.add_cli_args, which declares both options across | ||
| # the supported SGLang releases. | ||
| check_elastic_ep_backend( |
There was a problem hiding this comment.
The separately supported XPU image pins SGLang v0.5.11, while elastic EP was introduced in v0.5.16. Its parser therefore does not create elastic_ep_backend, so every ordinary XPU SGLang worker now fails with AttributeError during argument parsing even though it did not request mooncake. Read these optional fields defensively so the preflight remains inert on engines that do not support elastic EP.
🤖 AI Fix
Pass getattr(parsed_args, "elastic_ep_backend", None) and getattr(parsed_args, "enable_dp_attention", False) to the preflight check.
There was a problem hiding this comment.
Checked this one by reading the tagged source, and the premise does not hold. SGLang v0.5.11 does declare both options, so no AttributeError is possible on the XPU image.
Where v0.5.11 declares them
In python/sglang/srt/server_args.py at tag v0.5.11, both add_argument calls sit inside add_cli_args, which starts at line 4187:
574: elastic_ep_backend: Literal[None, "mooncake", "nixl"] = None
5692: parser.add_argument(
5693: "--elastic-ep-backend",
5696: choices=["none", "mooncake", "nixl"],
6096: parser.add_argument(
6097: "--enable-dp-attention",
args.py:371 builds the parser from ServerArgs.add_cli_args, so both attributes exist. Elastic EP is present in v0.5.11, not new in v0.5.16.
I did not run the XPU image. This is read from the tag the image builds from, container/context.yaml sglang.xpu.sglang_ref: v0.5.11.
|
|
||
|
|
||
| def _simulate_healthy_mooncake(monkeypatch): | ||
| """An image whose mooncake ProcessGroup extension loads and registers.""" |
There was a problem hiding this comment.
This docstring only expands _simulate_healthy_mooncake into a sentence; the helper name and its configured successful import/backend registrations already convey the intent.
🤖 AI Fix
Remove the _simulate_healthy_mooncake docstring.
|
|
||
|
|
||
| def test_accepts_mooncake_when_the_image_can_serve_it(monkeypatch): | ||
| """Negative control: a working image is not blocked.""" |
There was a problem hiding this comment.
The test name already states that a servable mooncake image is accepted, so this docstring adds no lasting constraint or rationale.
🤖 AI Fix
Remove the test_accepts_mooncake_when_the_image_can_serve_it docstring.
| # against a half-initialized engine, so it propagates. | ||
| except (ImportError, OSError) as _exc: | ||
| if isinstance(_exc, ModuleNotFoundError) and _exc.name == _name: | ||
| continue # the engine is simply not installed; nothing to report |
There was a problem hiding this comment.
The ModuleNotFoundError type and matching module name already establish that the engine itself is not installed; this trailing comment only narrates the continue and duplicates the surrounding exception-handling explanation.
🤖 AI Fix
Remove the trailing comment after continue.
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Verified review
Verdict: approve. The change fails fast, before the model is fetched, and the error names the real cause. I reproduced the pre-fix crash and the post-fix rejection in the image this pull request builds.
One P2 and one P3 below. Nothing blocking.
What I ran, and what the runs showed
Image: ...:f495b8f922ae599ddb6312c811b7534ed141f59d-sglang-runtime-test on one RTX 6000 Ada. sglang 0.5.18, torch 2.13.0+cu130, mooncake-transfer-engine-cuda13 0.3.13.post1.
Cherry-pick check: the added and removed lines of this pull request are byte-identical to commit 5296909. No drift.
Crash reproduced on the base tree. I moved every pg_2_*.so out of the mooncake package, then started a worker with --elastic-ep-backend mooncake --enable-dp-attention:
ImportError: Mooncake PG was not built against torch==2.13.0.
...
model_runner.py", line 397, in __init__
self.init_shared_mooncake_transfer_engine()
ImportError: Failed to import 'set_transfer_engine' from 'mooncake.pg'.
Please upgrade your 'mooncake-transfer-engine' installation to 0.3.11 or above.
Same command on the head tree stops at parse_args with the new diagnostic, before fetch_model and before the runtime connects to etcd.
| tree | mooncake | result |
|---|---|---|
| base | broken | starts, dies in the scheduler child process |
| head | broken | ValueError at args.py:446, no model work |
| base | healthy | proceeds |
| head | healthy | proceeds |
| head | healthy, --elastic-ep-backend unset or nccl |
proceeds |
Which module name the check demands: I ran the real resolver against the upstream sources of three SGLang releases. v0.5.11 and v0.5.16 give ('mooncake.ep',), v0.5.18 gives ('mooncake.pg',), and no SGLang at all gives both. That matches what each release imports.
Tests: 11 pass, and all 11 are selected by -m "unit and gpu_0 and pre_merge". Three mutations, each one killed a different test:
| mutation | failures |
|---|---|
| make the check return early always | 3 |
drop mooncake-cpu from the required set |
1 |
| make the resolver always return both names | 2 |
conftest change: with an engine that raises PermissionError on import, the base tree aborts the whole pytest session. The head tree collects 83 tests and reports the cause once.
Where verification stopped
I did not run the XPU lane. My answer on SGLang v0.5.11 comes from reading the tagged source, not from running that image.
I did not run a real multi-node elastic-EP deployment, so I did not exercise the --enable-dp-attention all-gather on the first forward pass.
| for module_name in _required_process_group_modules(): | ||
| try: | ||
| importlib.import_module(module_name) | ||
| return None |
There was a problem hiding this comment.
[P2] The probe can pass while the backend is still unusable. components/src/dynamo/sglang/elastic_ep_preflight.py:97. Importing the module is not the same as having the symbol SGLang imports from it. Please also check that set_transfer_engine is present, or say in the docstring that this case is out of scope.
Constructed case: probe passes, worker still dies at the same line
In the image this pull request builds, I deleted set_transfer_engine from mooncake/pg.py and left everything else in place. This is the shape of a mooncake older than 0.3.11, which upstream names in its own error text.
PREFLIGHT: PASSED
has set_transfer_engine: False
backends: {'mooncake-cpu': ('cpu',), 'mooncake': ('cuda',)}
The worker then died in the same place the check exists to move earlier:
model_runner.py", line 397, in __init__
self.init_shared_mooncake_transfer_engine()
ImportError: Failed to import 'set_transfer_engine' from 'mooncake.pg'.
Please upgrade your 'mooncake-transfer-engine' installation to 0.3.11 or above.
Control: with set_transfer_engine restored and the same command, the worker got past this point.
I am not proposing a patch. Which symbol the engine needs changes with the SGLang release, so a blanket check could refuse a working older image.
| lines = [ | ||
| "--elastic-ep-backend mooncake was requested, but the mooncake torch " | ||
| "ProcessGroup backend is not usable in this image. SGLang builds its " | ||
| "elastic-EP process groups from that backend after the model is " |
There was a problem hiding this comment.
[P3] The diagnostic states a cause that the pinned engine contradicts. components/src/dynamo/sglang/elastic_ep_preflight.py:218. The message tells the operator the groups are built "after the model is loaded". In the image this pull request builds, the failure lands before the weights are read. Please reword, or name the case where the failure really is late.
Measured call order in the shipped engine
model_runner.py in the image: init_shared_mooncake_transfer_engine() is called at line 397, maybe_init_elastic_ep() at line 648, and load_model() at line 651.
My reproduction died at line 397, about 6 seconds after start, with no weight-loading lines in the log.
The late path the text describes does exist for the --enable-dp-attention all-gather on the first forward pass. I did not exercise that path, so this is a wording point, not a claim that the late failure is impossible.
Overview:
Cherry-pick of #14461 to release/1.5.0.
Details:
Cherry-picks commit 5296909 from main, which stops SGLang workers crashing after model load when an unusable mooncake backend is configured.
Where should the reviewer start?
Same diff as #14461 on main.