chore(dynamo-run): Remove out=sglang|vllm|trtllm #1920
Conversation
grahamking
requested review from
a team,
GuanLuo,
PeaBrane,
alec-flowers,
biswapanda,
ishandhanani,
jthomson04,
kkranen,
nnshah1,
paulhendricks,
piotrm-nvidia,
ptarasiewiczNV,
rmccorm4,
ryanolson,
tanmayv25,
tedzhouhk,
tmonty12 and
whoisj
as code owners
|
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/get_started.md (1)
126-127: Documentation placeholder appropriately added.The TODO comment correctly indicates this section needs updating due to the removal of subprocess-based engine management. This helps maintain documentation accuracy during the transition period.
Would you like me to help draft updated documentation for this section that reflects the new workflow where users must start workers themselves?
launch/dynamo-run/src/lib.rs (1)
43-45: Update or remove the outdated TODO comment.This TODO comment references old subprocess handling that is no longer relevant after the removal of subprocess-based engines. The comment mentions "trtllm/sglang/vllm subprocess" which are no longer supported according to the PR objectives.
Consider updating this comment to reflect the current architecture or removing it entirely:
- // TODO: old, address this later: - // If `in=dyn` we want the trtllm/sglang/vllm subprocess to listen on that endpoint. - // If not, then the endpoint isn't exposed so we let LocalModel invent one. + // If `in=dyn` we want the engine to listen on that endpoint. + // If not, then the endpoint isn't exposed so we let LocalModel invent one.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
docs/get_started.md(1 hunks)docs/guides/dynamo_run.md(1 hunks)examples/cli/cli.py(1 hunks)examples/cli/sglang_inc.py(0 hunks)examples/cli/trtllm_inc.py(0 hunks)examples/cli/vllm_inc.py(0 hunks)launch/dynamo-run/src/flags.rs(0 hunks)launch/dynamo-run/src/lib.rs(2 hunks)launch/dynamo-run/src/main.rs(1 hunks)launch/dynamo-run/src/opt.rs(0 hunks)launch/dynamo-run/src/subprocess.rs(0 hunks)launch/dynamo-run/src/subprocess/sglang.rs(0 hunks)launch/dynamo-run/src/subprocess/sglang_inc.py(0 hunks)launch/dynamo-run/src/subprocess/trtllm.rs(0 hunks)launch/dynamo-run/src/subprocess/trtllm_config/sample.yaml(0 hunks)launch/dynamo-run/src/subprocess/trtllm_inc.py(0 hunks)launch/dynamo-run/src/subprocess/vllm.rs(0 hunks)launch/dynamo-run/src/subprocess/vllm_inc.py(0 hunks)launch/dynamo-run/src/subprocess/vllm_v1_inc.py(0 hunks)lib/llm/src/entrypoint/input/endpoint.rs(1 hunks)
💤 Files with no reviewable changes (14)
- launch/dynamo-run/src/subprocess/trtllm_config/sample.yaml
- launch/dynamo-run/src/subprocess/vllm.rs
- launch/dynamo-run/src/subprocess/sglang.rs
- launch/dynamo-run/src/subprocess.rs
- examples/cli/vllm_inc.py
- examples/cli/sglang_inc.py
- launch/dynamo-run/src/opt.rs
- launch/dynamo-run/src/subprocess/vllm_inc.py
- launch/dynamo-run/src/subprocess/sglang_inc.py
- launch/dynamo-run/src/flags.rs
- launch/dynamo-run/src/subprocess/trtllm.rs
- examples/cli/trtllm_inc.py
- launch/dynamo-run/src/subprocess/trtllm_inc.py
- launch/dynamo-run/src/subprocess/vllm_v1_inc.py
🧰 Additional context used
🧠 Learnings (3)
docs/guides/dynamo_run.md (2)
Learnt from: biswapanda
PR: ai-dynamo/dynamo#1412
File: lib/bindings/python/src/dynamo/runtime/logging.py:100-100
Timestamp: 2025-06-06T21:48:35.214Z
Learning: In the Dynamo codebase, BentoML has been completely removed from all executable code, with only documentation and attribution references remaining. The error_loggers configuration in lib/bindings/python/src/dynamo/runtime/logging.py should not include "bentoml" since those modules no longer exist.
Learnt from: nnshah1
PR: ai-dynamo/dynamo#1444
File: tests/fault_tolerance/utils/metrics.py:30-32
Timestamp: 2025-07-01T13:55:03.940Z
Learning: The `@dynamo_worker()` decorator in the dynamo codebase returns a wrapper that automatically injects the `runtime` parameter before calling the wrapped function. This means callers only need to provide the non-runtime parameters, while the decorator handles injecting the runtime argument automatically. For example, a function with signature `async def get_metrics(runtime, log_dir)` decorated with `@dynamo_worker()` can be called as `get_metrics(log_dir)` because the decorator wrapper injects the runtime parameter.
lib/llm/src/entrypoint/input/endpoint.rs (3)
Learnt from: PeaBrane
PR: ai-dynamo/dynamo#1285
File: lib/llm/src/kv_router/scoring.rs:58-63
Timestamp: 2025-05-30T06:38:09.630Z
Learning: In lib/llm/src/kv_router/scoring.rs, the user prefers to keep the panic behavior when calculating load_avg and variance with empty endpoints rather than adding guards for division by zero. They want the code to fail fast on this error condition.
Learnt from: PeaBrane
PR: ai-dynamo/dynamo#1392
File: lib/llm/src/kv_router/scoring.rs:35-46
Timestamp: 2025-06-05T01:02:15.318Z
Learning: In lib/llm/src/kv_router/scoring.rs, PeaBrane prefers panic-based early failure over Result-based error handling for the worker_id() method to catch invalid data early during development.
Learnt from: ryanolson
PR: ai-dynamo/dynamo#1093
File: lib/llm/src/block_manager/block/registry.rs:98-122
Timestamp: 2025-05-29T06:20:12.901Z
Learning: In lib/llm/src/block_manager/block/registry.rs, the background task spawned for handling unregister notifications uses detached concurrency by design. The JoinHandle is intentionally not stored as this represents a reasonable architectural tradeoff for a long-running cleanup task.
launch/dynamo-run/src/lib.rs (1)
Learnt from: PeaBrane
PR: ai-dynamo/dynamo#1285
File: lib/llm/src/kv_router/scoring.rs:58-63
Timestamp: 2025-05-30T06:38:09.630Z
Learning: In lib/llm/src/kv_router/scoring.rs, the user prefers to keep the panic behavior when calculating load_avg and variance with empty endpoints rather than adding guards for division by zero. They want the code to fail fast on this error condition.
🔇 Additional comments (7)
docs/guides/dynamo_run.md (1)
3-8: Well-placed warning banner for outdated documentation.The prominently displayed warning banner effectively communicates that the document is temporarily out of date due to ongoing changes. The formatting with horizontal rules makes it visually clear to users.
launch/dynamo-run/src/main.rs (1)
93-96: Clean implementation of engine restriction with helpful error messaging.The conditional check effectively prevents usage of the removed engines while providing clear guidance to users about alternative approaches. The error message is informative and the exit code is appropriate.
lib/llm/src/entrypoint/input/endpoint.rs (1)
83-83: Appropriate replacement with explicit assertion.The
unreachable!macro is a cleaner approach than a never-resolving future for this code path that should never be reached. The assertion message clearly explains why this case is unreachable.examples/cli/cli.py (1)
145-145: Excellent simplification by removing subprocess orchestration.The simplified
run_inputcall reflects the removal of complex subprocess management logic. This makes the CLI more straightforward and focused, eliminating the need for signal handling, cleanup functions, and subprocess lifecycle management.launch/dynamo-run/src/lib.rs (3)
68-68: Function call correctly updated to match new signature.The
engine_forfunction call has been properly updated to remove theflagsparameter, which is consistent with the subprocess removal.
80-84: Function signature properly simplified.The function signature has been correctly simplified:
- Removed the
flagsparameter (no longer needed without subprocess management)- Changed return type from tuple to
EngineConfig(no more subprocess futures to manage)This aligns well with the PR objective of removing subprocess spawning.
85-105: Match arms correctly simplified without subprocess logic.The match arms have been properly simplified to return
EngineConfigvariants directly. The removal of subprocess-based engines (sglang, vllm, trtllm) is clean and consistent with the PR objectives.The remaining engines (Dynamic, EchoFull, EchoCore, MistralRs, LlamaCpp) are handled correctly without any subprocess spawning logic.
|
This should be merged after the
|
|
|
grahamking
force-pushed
the
gk-separate-subprocess
branch
from
4f3d168 to
2b3f15b
Compare
grahamking
force-pushed
the
gk-separate-subprocess
branch
from
2b3f15b to
3bde57b
Compare
Instead of `dynamo-run` spawning them as a sub-process, we now ask the user to do it. In practice the worker is nearly always on a separate machine so this makes that more obvious. Part of the dynamo-run disaggregation.
grahamking
force-pushed
the
gk-separate-subprocess
branch
from
3bde57b to
189f25a
Compare
|
Hi @grahamking. I am new to Dynamo and currently following https://confluence.nvidia.com/display/DL/Dynamo+Onboarding+Page. Can you update line |
I don't think that's a publically visible page. This repo is up to date, start here: https://github.com/ai-dynamo/dynamo/tree/main/examples/basics/quickstart |
UX v2 has a smaller role for dynamo-run. Clarify that in the guide. - Remove sglang, vllm and trtllm engines. - Remove old KV-routing (vllm patch pre merge) instructions. - Adjust the Markdown header levels to reflect document organisation. Follow-up to #1920
Instead of
dynamo-runspawning them as a sub-process, we now ask the user to do it. In practice the worker is nearly always on a separate machine so this makes that more obvious.Part of the dynamo-run disaggregation.