[Bugfix] Add the check for a null VllmConfig#4749
[Bugfix] Add the check for a null VllmConfig#4749wangxiyuan merged 2 commits intovllm-project:mainfrom
Conversation
Signed-off-by: gcanlin <canlinguosdu@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request adds a null check for vllm_config in the is_vl_model function to prevent a potential AttributeError. The change is correct and improves the robustness of the code. However, the fix is incomplete as it doesn't check for vllm_config.model_config.hf_config before use, which could still lead to an AttributeError. I've added a comment with a suggestion to make the check more comprehensive. Additionally, a similar issue exists in the is_moe_model function which could also be addressed for consistency.
|
👋 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. |
### What this PR does / why we need it? In vllm-omni, we create the empty `VllmConfig`, which raised the null error in [`vllm-ascend/vllm_ascend/utils.py`](https://github.com/vllm-project/vllm-ascend/blob/a7f91079b8576a846f671c9e6923805e74e35c87/vllm_ascend/utils.py#L833). More details are [here](vllm-project/vllm-omni#208). ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? - vLLM version: v0.12.0 - vLLM main: vllm-project/vllm@ad32e3e Signed-off-by: gcanlin <canlinguosdu@gmail.com>
### What this PR does / why we need it? In vllm-omni, we create the empty `VllmConfig`, which raised the null error in [`vllm-ascend/vllm_ascend/utils.py`](https://github.com/vllm-project/vllm-ascend/blob/a7f91079b8576a846f671c9e6923805e74e35c87/vllm_ascend/utils.py#L833). More details are [here](vllm-project/vllm-omni#208). ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? - vLLM version: v0.12.0 - vLLM main: vllm-project/vllm@ad32e3e Signed-off-by: gcanlin <canlinguosdu@gmail.com> Signed-off-by: weijinqian_v1 <weijinqian@huawei.com>
### What this PR does / why we need it? In vllm-omni, we create the empty `VllmConfig`, which raised the null error in [`vllm-ascend/vllm_ascend/utils.py`](https://github.com/vllm-project/vllm-ascend/blob/a7f91079b8576a846f671c9e6923805e74e35c87/vllm_ascend/utils.py#L833). More details are [here](vllm-project/vllm-omni#208). ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? - vLLM version: v0.12.0 - vLLM main: vllm-project/vllm@ad32e3e Signed-off-by: gcanlin <canlinguosdu@gmail.com>
### What this PR does / why we need it? In vllm-omni, we create the empty `VllmConfig`, which raised the null error in [`vllm-ascend/vllm_ascend/utils.py`](https://github.com/vllm-project/vllm-ascend/blob/a7f91079b8576a846f671c9e6923805e74e35c87/vllm_ascend/utils.py#L833). More details are [here](vllm-project/vllm-omni#208). ### Does this PR introduce _any_ user-facing change? ### How was this patch tested? - vLLM version: v0.12.0 - vLLM main: vllm-project/vllm@ad32e3e Signed-off-by: gcanlin <canlinguosdu@gmail.com>
What this PR does / why we need it?
In vllm-omni, we create the empty
VllmConfig, which raised the null error invllm-ascend/vllm_ascend/utils.py. More details are here.Does this PR introduce any user-facing change?
How was this patch tested?