fix(sglang): add enable_trace to diffusion worker ServerArgs stub#8332
Conversation
worker ServerArgs stub Signed-off-by: Krishnan Prashanth <kprashanth@nvidia.com>
WalkthroughTwo files modified to handle the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/src/dynamo/sglang/request_handlers/handler_base.py (1)
147-147: Keep theenable_tracefallback in one documented place.
BaseWorkerHandler.__init__already callsBaseGenerativeHandler.__init__, so Line 702 repeats the same assignment. Since this defensivegetattr()is intentionally handling partialServerArgsstubs, add the rationale once near Line 147 and remove the duplicate.♻️ Proposed cleanup
""" self.config = config + # Some diffusion/video workers pass a minimal ServerArgs-like stub, so + # enable_trace may be absent even though real ServerArgs defines it. self.enable_trace = getattr(config.server_args, "enable_trace", False)self.serving_mode = config.serving_mode self.use_sglang_tokenizer = config.dynamo_args.use_sglang_tokenizer - self.enable_trace = getattr(config.server_args, "enable_trace", False) if engine is not None:As per coding guidelines, “NO defensive getattr() on known types” should be generally avoided, but is likely justified here because the repository uses minimal/partial
ServerArgsstubs; ensure the code comment or surrounding logic makes that rationale explicit.Also applies to: 702-702
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/src/dynamo/sglang/request_handlers/handler_base.py` at line 147, The duplicate defensive getattr assignment for enable_trace should be centralized and documented: remove the repeated assignment in BaseWorkerHandler.__init__ (the line at/around where enable_trace is set again) and keep the single getattr(config.server_args, "enable_trace", False) in BaseGenerativeHandler.__init__ (around the existing assignment at Line 147), adding a short comment explaining that this defensive getattr is intentional to support partial ServerArgs stubs; ensure all callers rely on the single documented assignment and run tests to confirm no regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@components/src/dynamo/sglang/request_handlers/handler_base.py`:
- Line 147: The duplicate defensive getattr assignment for enable_trace should
be centralized and documented: remove the repeated assignment in
BaseWorkerHandler.__init__ (the line at/around where enable_trace is set again)
and keep the single getattr(config.server_args, "enable_trace", False) in
BaseGenerativeHandler.__init__ (around the existing assignment at Line 147),
adding a short comment explaining that this defensive getattr is intentional to
support partial ServerArgs stubs; ensure all callers rely on the single
documented assignment and run tests to confirm no regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eb7ea119-3dd7-4a9d-a2cb-a055627f46ce
📒 Files selected for processing (2)
components/src/dynamo/sglang/args.pycomponents/src/dynamo/sglang/request_handlers/handler_base.py
) (#8361) Signed-off-by: Krishnan Prashanth <kprashanth@nvidia.com>
Overview:
The
SimpleNamespacestub created for image/video diffusion workers omittedenable_trace, causingBaseGenerativeHandler()to crash withAttributeErrorbefore health check. Add the field to the stub and usegetattr()defensively in bothBaseGenerativeHandlerandBaseWorkerHandler.Details:
BaseGenerativeHandler.__init__andBaseWorkerHandler.__init__Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Fixes DYN-2711
Summary by CodeRabbit