feat: sglang V4 (DeepSeek-V4) compatibility for decode handler - #8678
feat: sglang V4 (DeepSeek-V4) compatibility for decode handler#8678davilu-nvidia wants to merge 1 commit into
Conversation
Add use_sglang_tokenizer, _get_guided_decoding_params, _resolve_lora, _session_kwargs stubs to DecodeWorkerHandler for sglang V4 (DeepSeek-V4) disaggregated serving on H20 GPUs. Signed-off-by: David Lu <davilu@nvidia.com>
|
👋 Hi davilu-nvidia! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
Bot
added
feat
external-contribution
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/src/dynamo/sglang/request_handlers/llm/decode_handler.py (1)
324-339:⚠️ Potential issue | 🟡 MinorEither restore
return_routed_expertsor delete the dead plumbing.Commenting out the kwarg makes
enable_return_routed_expertsa no-op, and Ruff already reports F841 on the now-unused local. If routed-expert payloads are still expected downstream, forward the flag in bothasync_generatecalls; otherwise remove the assignment and the commented-out lines entirely.🧹 Suggested fix
- return_routed_experts = getattr( - self.config.server_args, "enable_return_routed_experts", False - ) ... - # return_routed_experts=return_routed_experts,Also applies to: 360-374
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/src/dynamo/sglang/request_handlers/llm/decode_handler.py` around lines 324 - 339, The commented-out return_routed_experts kwarg in the async_generate call makes enable_return_routed_experts a no-op and leaves an unused local; either re-enable forwarding of the flag by adding return_routed_experts=return_routed_experts to both async_generate calls (the one around decode and the other at lines ~360-374) so routed-expert payloads propagate, or remove the enable_return_routed_experts/local return_routed_experts assignment and the commented-out kwarg lines entirely (and any downstream handling that expects routed experts) to eliminate dead plumbing and the F841 unused-variable warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/src/dynamo/sglang/request_handlers/llm/decode_handler.py`:
- Around line 69-70: The handler currently overwrites self.use_sglang_tokenizer
= False after calling BaseWorkerHandler.__init__(), which breaks the
config-driven request-parsing state; update DecodeHandler so the tokenizer flag
is set consistently by either assigning use_sglang_tokenizer from the config
before calling BaseWorkerHandler.__init__() or, if it must be forced, rebuild
any dependent state (the request-parsing state initialized in
BaseWorkerHandler.__init__) immediately after setting it; reference the
DecodeHandler.__init__ method and BaseWorkerHandler.__init__ and ensure no
post-init hard-coded assignment leaves the handler inconsistent.
- Around line 78-91: The three static method stubs _get_guided_decoding_params,
_resolve_lora, and _session_kwargs in decode_handler.py override parent
implementations and return incorrect types/lose access to instance methods
(e.g., self.lora_id_for_name) causing LoRA resolution and guided decoding to
break; remove these three method definitions so the class will inherit the
correct implementations from LoraMixin and BaseGenerativeHandler, restoring
proper Optional[str] return behavior for _resolve_lora and the routing/guided
decoding logic for _session_kwargs and _get_guided_decoding_params.
---
Outside diff comments:
In `@components/src/dynamo/sglang/request_handlers/llm/decode_handler.py`:
- Around line 324-339: The commented-out return_routed_experts kwarg in the
async_generate call makes enable_return_routed_experts a no-op and leaves an
unused local; either re-enable forwarding of the flag by adding
return_routed_experts=return_routed_experts to both async_generate calls (the
one around decode and the other at lines ~360-374) so routed-expert payloads
propagate, or remove the enable_return_routed_experts/local
return_routed_experts assignment and the commented-out kwarg lines entirely (and
any downstream handling that expects routed experts) to eliminate dead plumbing
and the F841 unused-variable warning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1313256-84ea-4b3e-9b9b-3a75735bc42c
📒 Files selected for processing (1)
components/src/dynamo/sglang/request_handlers/llm/decode_handler.py
| # H20 patch: set use_sglang_tokenizer from config | ||
| self.use_sglang_tokenizer = False # disagg mode |
There was a problem hiding this comment.
Don't hard-code use_sglang_tokenizer after super().__init__().
BaseWorkerHandler.__init__() already used the config value to build request-parsing state, so forcing this field to False here makes the handler internally inconsistent and bypasses the new flag for aggregated serving. If decode mode truly needs a fixed tokenizer setting, apply it before super().__init__() or rebuild the dependent state afterward.
🛠️ Suggested fix
- # H20 patch: set use_sglang_tokenizer from config
- self.use_sglang_tokenizer = False # disagg mode
+ # Preserve the configured tokenizer mode; the base class already
+ # initialized its dependent state from this value.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # H20 patch: set use_sglang_tokenizer from config | |
| self.use_sglang_tokenizer = False # disagg mode | |
| # Preserve the configured tokenizer mode; the base class already | |
| # initialized its dependent state from this value. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/src/dynamo/sglang/request_handlers/llm/decode_handler.py` around
lines 69 - 70, The handler currently overwrites self.use_sglang_tokenizer =
False after calling BaseWorkerHandler.__init__(), which breaks the config-driven
request-parsing state; update DecodeHandler so the tokenizer flag is set
consistently by either assigning use_sglang_tokenizer from the config before
calling BaseWorkerHandler.__init__() or, if it must be forced, rebuild any
dependent state (the request-parsing state initialized in
BaseWorkerHandler.__init__) immediately after setting it; reference the
DecodeHandler.__init__ method and BaseWorkerHandler.__init__ and ensure no
post-init hard-coded assignment leaves the handler inconsistent.
| @staticmethod | ||
| def _get_guided_decoding_params(guided_decoding=None): | ||
| """Stub for guided decoding - not needed for E2E test.""" | ||
| return {} | ||
|
|
||
| @staticmethod | ||
| def _resolve_lora(request): | ||
| """Stub for LoRA resolution.""" | ||
| return {} | ||
|
|
||
| @staticmethod | ||
| def _session_kwargs(request): | ||
| """Stub for session kwargs.""" | ||
| return {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n components/src/dynamo/sglang/request_handlers/llm/decode_handler.py | head -120Repository: ai-dynamo/dynamo
Length of output: 4903
🏁 Script executed:
# Check if there's a parent class definition in the file
rg "class.*\(" components/src/dynamo/sglang/request_handlers/llm/decode_handler.py -A 3Repository: ai-dynamo/dynamo
Length of output: 214
🏁 Script executed:
# Look for imports to understand inheritance
rg "^from|^import" components/src/dynamo/sglang/request_handlers/llm/decode_handler.py | head -20Repository: ai-dynamo/dynamo
Length of output: 589
🏁 Script executed:
# Find and examine the parent class BaseWorkerHandler
find . -name "handler_base.py" -path "*/sglang/*" | head -5Repository: ai-dynamo/dynamo
Length of output: 124
🏁 Script executed:
# Search for the parent class and its method implementations
rg "def _get_guided_decoding_params|def _resolve_lora|def _session_kwargs" --type py -B 2 -A 5Repository: ai-dynamo/dynamo
Length of output: 4948
🏁 Script executed:
# Search for calls to these stub methods to understand expected contract
rg "_get_guided_decoding_params|_resolve_lora|_session_kwargs" --type py -B 2 -A 2Repository: ai-dynamo/dynamo
Length of output: 8601
🏁 Script executed:
# Get more context from the base class implementation
sed -n '1,250p' components/src/dynamo/sglang/request_handlers/handler_base.py | grep -A 20 "_resolve_lora"Repository: ai-dynamo/dynamo
Length of output: 42
🏁 Script executed:
# Check the full implementations in base class
sed -n '1,400p' components/src/dynamo/sglang/request_handlers/handler_base.py | grep -A 15 "def _session_kwargs"Repository: ai-dynamo/dynamo
Length of output: 42
🏁 Script executed:
# Check the full implementation of _get_guided_decoding_params in base
sed -n '1,400p' components/src/dynamo/sglang/request_handlers/handler_base.py | grep -A 15 "def _get_guided_decoding_params"Repository: ai-dynamo/dynamo
Length of output: 42
🏁 Script executed:
# Get the size and read the handler_base.py file
wc -l components/src/dynamo/sglang/request_handlers/handler_base.pyRepository: ai-dynamo/dynamo
Length of output: 127
🏁 Script executed:
# Read the handler_base.py file to find the method implementations
cat components/src/dynamo/sglang/request_handlers/handler_base.py | head -500Repository: ai-dynamo/dynamo
Length of output: 18776
🏁 Script executed:
# Search for the implementations of these methods in handler_base.py
grep -n "def _resolve_lora\|def _session_kwargs\|def _get_guided_decoding_params" components/src/dynamo/sglang/request_handlers/handler_base.pyRepository: ai-dynamo/dynamo
Length of output: 256
🏁 Script executed:
# Get the implementation of _resolve_lora method starting from the line
sed -n '780,810p' components/src/dynamo/sglang/request_handlers/handler_base.pyRepository: ai-dynamo/dynamo
Length of output: 1122
🏁 Script executed:
# Get the implementation of _session_kwargs method
sed -n '810,850p' components/src/dynamo/sglang/request_handlers/handler_base.pyRepository: ai-dynamo/dynamo
Length of output: 1647
🏁 Script executed:
# Get the implementation of _get_guided_decoding_params method
sed -n '850,900p' components/src/dynamo/sglang/request_handlers/handler_base.pyRepository: ai-dynamo/dynamo
Length of output: 2241
🏁 Script executed:
# Get the actual implementations starting from the correct line numbers
sed -n '302,320p' components/src/dynamo/sglang/request_handlers/handler_base.pyRepository: ai-dynamo/dynamo
Length of output: 759
🏁 Script executed:
# Get _session_kwargs implementation
sed -n '1044,1070p' components/src/dynamo/sglang/request_handlers/handler_base.pyRepository: ai-dynamo/dynamo
Length of output: 1334
🏁 Script executed:
# Get _get_guided_decoding_params implementation
sed -n '1057,1080p' components/src/dynamo/sglang/request_handlers/handler_base.pyRepository: ai-dynamo/dynamo
Length of output: 1036
🏁 Script executed:
# Verify the calls to these methods in DecodeWorkerHandler and check if they're used correctly
sed -n '78,300p' components/src/dynamo/sglang/request_handlers/llm/decode_handler.py | grep -A 5 "_resolve_lora\|_session_kwargs\|_get_guided_decoding_params"Repository: ai-dynamo/dynamo
Length of output: 1061
🏁 Script executed:
# Check return type annotations and what lora_path is used for
grep -n "lora_path" components/src/dynamo/sglang/request_handlers/llm/decode_handler.py | head -20Repository: ai-dynamo/dynamo
Length of output: 312
🏁 Script executed:
# Check if DecodeWorkerHandler uses LoraMixin
grep -n "class DecodeWorkerHandler\|LoraMixin\|_init_lora_tracking" components/src/dynamo/sglang/request_handlers/llm/decode_handler.pyRepository: ai-dynamo/dynamo
Length of output: 109
🏁 Script executed:
# Check BaseWorkerHandler class definition and its mixins
sed -n '1100,1150p' components/src/dynamo/sglang/request_handlers/handler_base.py | grep -A 20 "class BaseWorkerHandler"Repository: ai-dynamo/dynamo
Length of output: 42
🏁 Script executed:
# Search for BaseWorkerHandler class definition in the full file
grep -n "^class BaseWorkerHandler" components/src/dynamo/sglang/request_handlers/handler_base.pyRepository: ai-dynamo/dynamo
Length of output: 153
Remove these method stubs—they return incorrect types and break LoRA resolution.
The stubs override working parent methods from LoraMixin and BaseGenerativeHandler. Specifically:
_resolve_lora()returns{}instead ofOptional[str], causingif lora_path:to evaluate as truthy even for empty dicts; it also cannot accessself.lora_id_for_namebecause it is@staticmethod._session_kwargs()and_get_guided_decoding_params()drop routing and guided decoding logic.
Delete these three methods to inherit the parent implementations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/src/dynamo/sglang/request_handlers/llm/decode_handler.py` around
lines 78 - 91, The three static method stubs _get_guided_decoding_params,
_resolve_lora, and _session_kwargs in decode_handler.py override parent
implementations and return incorrect types/lose access to instance methods
(e.g., self.lora_id_for_name) causing LoRA resolution and guided decoding to
break; remove these three method definitions so the class will inherit the
correct implementations from LoraMixin and BaseGenerativeHandler, restoring
proper Optional[str] return behavior for _resolve_lora and the routing/guided
decoding logic for _session_kwargs and _get_guided_decoding_params.
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
|
This PR has been closed due to inactivity. If you believe this PR is still relevant, please feel free to reopen it with additional context or information. |
1 participant
Summary
Compatibility fixes for DecodeWorkerHandler to support sglang V4 (DeepSeek-V4-Flash) disaggregated serving on H20 GPUs.
Overlap with #8671
return_routed_expertsparameter (same fix as [INVESTIGATE] sglang + DSv4: kwarg workaround breaks live — needs diagnosis #8671)Additional fixes beyond #8671
use_sglang_tokenizerflag: Replaces the removedskip_tokenizer_initattribute. sglang V4 no longer exposesskip_tokenizer_initon the engine; without this flag, all tokenizer-dependent code paths break._get_guided_decoding_params()stub: sglang V4's request flow calls this method for guided decoding support. Missing method causesAttributeErrorat runtime._resolve_lora()stub: sglang V4 invokes LoRA resolution per request. Without the stub, requests fail withAttributeError._session_kwargs()stub: sglang V4 passes session-level kwargs toasync_generate. Missing method breaks the generate call.lora_pathpass-through: Forwards resolved LoRA path toengine.async_generate()in both disaggregated and aggregated code paths.stop_token_idsmerging: Mergesstop_token_idsandstop_token_ids_hiddenfrom stop_conditions, fixing incomplete stop condition handling.Verified
Signed-off-by: David Lu davilu@nvidia.com