fix: adjust usage of vLLM for deprecation of 'disable_log_requests'#4659
Conversation
|
👋 Hi hypdeb! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
WalkthroughThis pull request renames the vLLM configuration parameter from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/src/dynamo/vllm/args.py (1)
450-459: Core vLLM defaults now aligned withenable_log_requestsUpdating the Dynamo vLLM defaults to set
"enable_log_requests": Falsekeeps the prior “turn off request logging by default” behavior while matching the new vLLM configuration field name. The overwrite loop still enforces these defaults over whatever came fromAsyncEngineArgs, which is consistent with the previous design.If you intend to eventually allow operators to control request logging purely via vLLM CLI flags, you might later relax this to only set
enable_log_requestswhen it isNoneor unset, mirroring howenable_prefix_cachingis handled earlier.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/src/dynamo/vllm/args.py(1 hunks)components/src/dynamo/vllm/main.py(1 hunks)examples/multimodal/components/worker.py(1 hunks)examples/multimodal/utils/args.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (3)
components/src/dynamo/vllm/main.py (1)
231-237: AsyncLLM call correctly updated to useenable_log_requestsWiring
enable_log_requests=engine_args.enable_log_requestsintoAsyncLLM.from_vllm_configis consistent with the new vLLM API and with your updated defaults (which now setenable_log_requests=False).Please confirm against the vLLM version you target that:
AsyncLLM.from_vllm_config(...)accepts anenable_log_requestskeyword, andAsyncEngineArgsexposes anenable_log_requestsattribute.If either name differs by version, you may need a small compatibility shim here.
examples/multimodal/components/worker.py (1)
141-147: Multimodal worker correctly switched toenable_log_requestsThe multimodal worker’s
AsyncLLM.from_vllm_configcall now forwardsenable_log_requests=self.engine_args.enable_log_requests, matching both the core worker path and your updated defaults inexamples.multimodal.utils.args.overwrite_args.Please verify against your target vLLM version that this parameter name and the corresponding
engine_argsattribute are present and that there are no remaining call sites still using the deprecateddisable_log_requestskeyword.examples/multimodal/utils/args.py (1)
148-171: Migration toenable_log_requestsin defaults is semantically correctSwitching the Dynamo multimodal defaults from
disable_log_requests=Truetoenable_log_requests=Falsepreserves the "no per‑request logging by default" behavior while aligning with the new vLLM API. Thehasattr+setattrpattern maintains compatibility withAsyncEngineArgsand fails fast if the expected field is missing.As a follow-up, confirm there are no lingering references to
disable_log_requestselsewhere in the codebase to ensure a complete migration.
rmccorm4
left a comment
There was a problem hiding this comment.
I think we can remove the setting with the new inverted behavior as described here: #4659 (comment)
But approving to avoid blocking a fix for incompatibility with ToT vLLM
|
/ok to test 7041619 |
…i-dynamo#4659) Co-authored-by: Ryan McCormick <rmccormick@nvidia.com>
Overview:
In vllm-project/vllm#29402,
disable_log_requestshas been removed. This results in a crash when using Dynamo with the ToT from vLLM, and will result in the same crash when using it with the upcoming 0.12 release.This PR addresses this by swapping the usages of
disable_log_requestsby the corresponding usage ofenable_log_requests.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.