fix(python): add router arg fallback disable flag#1072
fix(python): add router arg fallback disable flag#1072gongwei-130 wants to merge 4 commits intomainfrom
Conversation
Signed-off-by: gongwei-130 <weigong28@gmail.com>
gongwei-130
requested review from
CatherineSue,
key4ng and
slin1237
as code owners
github-actions
bot
added
python-bindings
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds an optional CLI flag Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea8ee2ee66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| elif ( | ||
| not disable_arg_fallback | ||
| and attr.name in cli_args_dict | ||
| and cli_args_dict[attr.name] not in (None, "") | ||
| ): |
There was a problem hiding this comment.
With --router-disable-arg-fallback, this branch blocks all unprefixed values, but serve builds router args with use_router_prefix=True and exclude_host_port=True, so router host/port can only come from unprefixed --host/--port. In that mode, enabling the new flag makes the router ignore the configured serve endpoint and silently fall back to RouterArgs defaults (0.0.0.0:30000), which can bind the gateway to the wrong address/port. Keep host/port fallback (or provide prefixed host/port args) when this flag is set.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new --router-disable-arg-fallback CLI argument, allowing users to explicitly disable the fallback mechanism where router arguments would otherwise inherit values from non-prefixed backend arguments. The changes include adding the argument definition, modifying the argument parsing logic to incorporate this new flag, and adding comprehensive unit tests to validate both the default fallback behavior and the disabled fallback behavior. Feedback from the review suggests minor improvements for code clarity and conciseness, such as removing a redundant bool() cast and simplifying SimpleNamespace initializations in the tests.
I am having trouble creating individual review comments. Click here to see my feedback.
bindings/python/src/smg/router_args.py (1115-1117)
The bool() cast is redundant here. The action="store_true" in argparse ensures that the value of router_disable_arg_fallback in the namespace is already a boolean (True if the flag is present, False otherwise). You can simplify this by removing the bool() call.
disable_arg_fallback = cli_args_dict.get(f"{prefix}disable_arg_fallback", False)bindings/python/tests/test_arg_parser.py (411-425)
The SimpleNamespace object is initialized with many attributes that are not relevant to this test case, making the test verbose and harder to read. You can simplify it by only including the attributes that are actually used to test the fallback logic (router_model_path, model_path, router_tokenizer_path, tokenizer_path, and router_disable_arg_fallback). The from_cli_args method is robust enough to handle missing attributes in the namespace.
args = SimpleNamespace(
router_model_path=None,
router_disable_arg_fallback=False,
model_path="backend/model",
router_tokenizer_path=None,
tokenizer_path="backend/tokenizer",
)bindings/python/tests/test_arg_parser.py (434-448)
Similar to the test above, the SimpleNamespace object here is initialized with many attributes that are not relevant to this test case. This makes the test verbose and harder to read. You can simplify it by only including the attributes that are actually used to test disabling the fallback logic.
args = SimpleNamespace(
router_model_path=None,
router_disable_arg_fallback=True,
model_path="backend/model",
router_tokenizer_path=None,
tokenizer_path="backend/tokenizer",
)Signed-off-by: gongwei-130 <weigong28@gmail.com>
gongwei-130
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96c5e15e09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| not disable_arg_fallback | ||
| and attr.name in cli_args_dict | ||
| and cli_args_dict[attr.name] not in (None, "") |
There was a problem hiding this comment.
Disabling fallback here also drops host/port inheritance even though smg serve/launch_server register router args with exclude_host_port=True (bindings/python/src/smg/serve.py lines 540/554 and bindings/python/src/smg/launch_server.py line 161), so the router has no --router-host/--router-port source. With --router-disable-arg-fallback, RouterArgs.from_cli_args no longer reads unprefixed --host/--port, and the router silently reverts to dataclass defaults (0.0.0.0:30000) instead of the serve endpoint requested via CLI, which can bind the gateway to the wrong address/port.
Useful? React with 👍 / 👎.
2 participants
Summary
--router-disable-arg-fallbackforsmg serve--router-*flagsWhy this is needed
In HTTP mode,
--tool-call-parserand--reasoning-parserare meant for the backend engine. But SMG router could pick them up via argument fallback, which can cause parser-related errors. This flag lets us prevent router fallback so backend-only parser flags stay backend-only.Testing
--router-disable-arg-fallbackis accepted insmg serveSummary by CodeRabbit
New Features
--router-disable-arg-fallbackthat, when enabled, prevents router-specific settings from inheriting unset values from backend configuration.Tests