[P/D] Check wildcard address for layerwise connector - #7389
Conversation
Signed-off-by: liziyu <liziyu16@huawei.com>
Summary of ChangesHello, 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 enhances the robustness of the layerwise connector by explicitly disallowing the use of loopback addresses for its host configuration. This change prevents potential misconfigurations or unintended behavior when setting up the connector, ensuring it operates with valid network endpoints. The modification includes a dedicated function for loopback address detection and integrates this check into the argument parsing process. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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
|
![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 validation check to prevent the use of loopback or unspecified addresses for the layerwise connector host. This is achieved by adding a new function, is_loopback_address, and using it to validate the --host argument during argument parsing. My review focuses on improving the clarity and correctness of this new validation function by suggesting a rename and a more concise implementation that better utilizes the ipaddress library.
| def is_loopback_address(host): | ||
| """Check if it is a loopback address""" | ||
| # Localhost | ||
| if host in ['localhost', 'localhost.localdomain', 'local']: | ||
| return True | ||
|
|
||
| # 0.0.0.0 | ||
| if host in ['0.0.0.0', '::']: | ||
| return True | ||
|
|
||
| # Loopback address | ||
| try: | ||
| ip = ipaddress.ip_address(host) | ||
| return ip.is_loopback | ||
| except ValueError: | ||
| # If not a valid IP address, it may be a domain name, allow it to pass | ||
| return False |
There was a problem hiding this comment.
The function is_loopback_address incorrectly includes checks for unspecified addresses (e.g., 0.0.0.0, ::), which are distinct from loopback addresses. This makes the function name and docstring misleading. The implementation can be simplified and made more accurate by using ip.is_unspecified from the ipaddress module.
I suggest renaming the function to is_loopback_or_unspecified_address and updating its implementation for clarity and correctness. Please also update the call to this function and the corresponding error message on lines 292-293.
| def is_loopback_address(host): | |
| """Check if it is a loopback address""" | |
| # Localhost | |
| if host in ['localhost', 'localhost.localdomain', 'local']: | |
| return True | |
| # 0.0.0.0 | |
| if host in ['0.0.0.0', '::']: | |
| return True | |
| # Loopback address | |
| try: | |
| ip = ipaddress.ip_address(host) | |
| return ip.is_loopback | |
| except ValueError: | |
| # If not a valid IP address, it may be a domain name, allow it to pass | |
| return False | |
| def is_loopback_or_unspecified_address(host): | |
| """Check if a host is a loopback or unspecified address.""" | |
| if host in ('localhost', 'localhost.localdomain', 'local'): | |
| return True | |
| try: | |
| ip = ipaddress.ip_address(host) | |
| return ip.is_loopback or ip.is_unspecified | |
| except ValueError: | |
| # If not a valid IP address, it may be a domain name, allow it to pass | |
| return False |
|
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. |
liziyu179
changed the title
cf8492f to
05f7472
Compare
liziyu179
changed the title
05f7472 to
65fe230
Compare
### What this PR does / why we need it? Check wildcard address address for layerwise connector - vLLM version: v0.17.0 - vLLM main: vllm-project/vllm@4034c3d --------- Signed-off-by: liziyu <liziyu16@huawei.com>
…to qwen3next_graph * 'main' of https://github.com/vllm-project/vllm-ascend: (94 commits) [bugfix] Fixed the error issue when overlaying MTP and full decode on DSV3.1 C8. (vllm-project#7571) [eagle3][pcp] fix acceptance rate for eagle3 and pcp enabled (vllm-project#7549) [bugfix][CI] fix '_OpNamespace' 'vllm' object has no attribute 'qkv_rmsnorm_rope' (vllm-project#7620) [Nightly] Nightly pre-build image (vllm-project#7388) [Bugfix]Fix deepseek 3.2 C8 precision by rotary tensor (vllm-project#7537) adapt to main2main for model runner v2 (vllm-project#7578) [Patch] Fix balance scheduling (vllm-project#7611) [310P]fused recurrent gated delta rule pytorch core and ut (vllm-project#7398) [CI] refine issue triage rules, wan regex and update stale setting (vllm-project#7531) [Lint]Add lint hooks for clang-format, shellcheck, forbidden imports, and boolean context manager checks (vllm-project#7511) [doc] add enable_sparse_c8 option in configuration options (vllm-project#7600) lower log level in PD Disaggregation (vllm-project#7589) [model_runner_v2]:optimize the performance of the _compute_slot_mappings_kernel (vllm-project#7575) [Feat][SP] Suport SP for VL MoE models (vllm-project#7044) Fix Qwen3Next CI Config (vllm-project#7561) [Feat] Add npugraph_ex enablement logging (vllm-project#7574) [UT] Align input arguments with Ascend(Yarn)RotaryEmbedding with vLLM and add ut (vllm-project#7358) [P/D] Check wildcard address for layerwise connector (vllm-project#7389) [P/D] [Bugfix] fix mooncake layerconnector dead when update_decoder_info fail (vllm-project#7514) [BugFix][P/D] fix padding error on FullGraph mode && fix layerwise connector mamba accuracy (vllm-project#7506) ...
### What this PR does / why we need it? Check wildcard address address for layerwise connector - vLLM version: v0.17.0 - vLLM main: vllm-project/vllm@4034c3d --------- Signed-off-by: liziyu <liziyu16@huawei.com>
### What this PR does / why we need it? Check wildcard address address for layerwise connector - vLLM version: v0.17.0 - vLLM main: vllm-project/vllm@4034c3d --------- Signed-off-by: liziyu <liziyu16@huawei.com>
### What this PR does / why we need it? Check wildcard address address for layerwise connector - vLLM version: v0.17.0 - vLLM main: vllm-project/vllm@4034c3d --------- Signed-off-by: liziyu <liziyu16@huawei.com>
### What this PR does / why we need it? Check wildcard address address for layerwise connector - vLLM version: v0.17.0 - vLLM main: vllm-project/vllm@4034c3d --------- Signed-off-by: liziyu <liziyu16@huawei.com> Signed-off-by: nanxing <1014662416@qq.com>
### What this PR does / why we need it? Check wildcard address address for layerwise connector - vLLM version: v0.17.0 - vLLM main: vllm-project/vllm@4034c3d --------- Signed-off-by: liziyu <liziyu16@huawei.com>
### What this PR does / why we need it? Check wildcard address address for layerwise connector - vLLM version: v0.17.0 - vLLM main: vllm-project/vllm@4034c3d --------- Signed-off-by: liziyu <liziyu16@huawei.com>
4 participants
What this PR does / why we need it?
Check wildcard address address for layerwise connector
Does this PR introduce any user-facing change?
How was this patch tested?