Repository navigation
fix(serve): harden CLI arg filtering and config error handling - #483
Conversation
- Fix _filter_backend_args to handle --key=value syntax by splitting on '=' before matching against filter list. Previously only exact string match worked, so --model=/tmp/m would not be filtered when --model was in the filter list, causing duplicate args in worker commands. - Fix _get_tp_size to let FileNotFoundError and yaml.YAMLError propagate instead of catching all exceptions and silently falling back to tp=1. A missing or malformed --config file is a real misconfiguration that should fail loudly, not silently launch with wrong GPU allocation. - Fix VllmWorkerLauncher docstring that still said "gRPC mode only" after HTTP mode support was added. - Fix _filter_backend_args docstring that incorrectly referenced vLLM. - Remove stray print(cmd) left in test_build_command. - Add TestFilterBackendArgs covering --key value, --key=value, multiple keys, empty args, and no-filter-args cases. - Add test for malformed YAML config raising yaml.YAMLError. - Update config error test to expect FileNotFoundError propagation. - Update missing-keys test to verify warning message content. Signed-off-by: Simo Lin <simo.lin@oracle.com>
Summary of ChangesHello @slin1237, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses critical issues in the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughThis PR enhances argument filtering logic and error handling in the vLLM worker launcher configuration. Changes include more robust filtering of launcher-set arguments supporting both Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request provides good improvements to the serve CLI. It fixes a bug in argument filtering to correctly handle --key=value syntax and hardens error handling for config file parsing by propagating errors instead of silently failing. The accompanying test changes are thorough and correctly verify the new behavior. I have one suggestion to further harden the argument filtering logic to make it more robust against future changes.
Summary
Fixes bugs and hardens error handling in the serve CLI introduced by #460.
Refs: #460
What changed
bindings/python/src/smg/serve.py:_filter_backend_args: Split on=before matching against filter list so--key=valuesyntax is correctly filtered (previously only--key valueworked)_get_tp_size: File I/O and YAML parse errors now propagate instead of being silently caught by a broadexcept Exception. Missing/malformed--configfiles fail loudly instead of silently using tp=1_filter_backend_argsdocstring: Fixed incorrect reference to "vLLM's grpc_server"VllmWorkerLauncherdocstring: Removed stale "(gRPC mode only)" since HTTP mode is now supportedbindings/python/tests/test_serve.py:TestFilterBackendArgsclass (5 tests) covering--key value,--key=value, multiple keys, empty args, and no-filter casestest_get_tp_size_from_config_malformed_yaml_raises— verifiesyaml.YAMLErrorpropagationtest_get_tp_size_from_config_read_fails_raises— expectsFileNotFoundErrorinstead of silent fallbacktest_get_tp_size_from_config_empty_or_no_keys_returns_default— verifies warning message contentprint(cmd)debug statementWhy
--key=valuebug: If a user passed--model=/tmp/model, the filter compared the whole string against"--model"— no match. The arg leaked through unfiltered, causing duplicate--modelin the worker command.Silent tp=1 fallback: The old code caught all exceptions from config parsing and fell back to tp_size=1. A missing config file or malformed YAML would silently launch workers with wrong GPU allocation — potentially causing OOM or wasting resources.
Test plan
python3 -m pytest bindings/python/tests/test_serve.py -v— 101 passedSummary by CodeRabbit
Bug Fixes
Documentation