[CI] fix UT error. #2644
[CI] fix UT error. #2644
Conversation
Signed-off-by: weijinqian_v1 <weijinqian@huawei.com>
|
👋 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. |
![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 fixes a unit test error by adapting the creation of Request objects to handle API changes across different vllm versions. The changes introduce conditional logic based on the vllm version. My review focuses on improving the maintainability of this new logic by refactoring duplicated code and making the version checks more extensible. Both changes suffer from significant code duplication which can be addressed by extracting common parameters.
| if vllm_version_is("0.10.1.1") or vllm_version_is("0.10.1"): | ||
| request = Request(request_id=f"{i}", | ||
| prompt_token_ids=[i] * num_tokens, | ||
| sampling_params=sampling_params, | ||
| multi_modal_kwargs=None, | ||
| multi_modal_placeholders=None, | ||
| multi_modal_hashes=None, | ||
| eos_token_id=EOS_TOKEN_ID, | ||
| pooling_params=None, | ||
| block_hasher=get_request_block_hasher( | ||
| block_size, hash_fn)) | ||
| else: | ||
| request = Request(request_id=f"{i}", | ||
| prompt_token_ids=[i] * num_tokens, | ||
| sampling_params=sampling_params, | ||
| eos_token_id=EOS_TOKEN_ID, | ||
| pooling_params=None, | ||
| block_hasher=get_request_block_hasher( | ||
| block_size, hash_fn)) |
There was a problem hiding this comment.
There is significant code duplication between the if and else blocks when creating the Request object. This can make the code harder to maintain and prone to errors if one branch is updated but the other is not. You can refactor this by extracting the common arguments into a dictionary to reduce redundancy. The version check can also be made more concise and extensible.
request_kwargs = {
"request_id": f"{i}",
"prompt_token_ids": [i] * num_tokens,
"sampling_params": sampling_params,
"eos_token_id": EOS_TOKEN_ID,
"pooling_params": None,
"block_hasher": get_request_block_hasher(block_size, hash_fn),
}
if any(vllm_version_is(v) for v in ["0.10.1.1", "0.10.1"]):
request_kwargs.update({
"multi_modal_kwargs": None,
"multi_modal_placeholders": None,
"multi_modal_hashes": None,
})
request = Request(**request_kwargs)| if vllm_version_is("0.10.1.1") or vllm_version_is("0.10.1"): | ||
| req = Request( | ||
| request_id=f"id-{request_id}", | ||
| prompt_token_ids=prompt_token_ids, | ||
| sampling_params=sampling_params, | ||
| multi_modal_kwargs=None, | ||
| multi_modal_placeholders=None, | ||
| multi_modal_hashes=None, | ||
| pooling_params=[], | ||
| eos_token_id=EOS_TOKEN_ID, | ||
| block_hasher=block_hasher, | ||
| ) | ||
| else: | ||
| req = Request( | ||
| request_id=f"id-{request_id}", | ||
| prompt_token_ids=prompt_token_ids, | ||
| sampling_params=sampling_params, | ||
| pooling_params=[], | ||
| eos_token_id=EOS_TOKEN_ID, | ||
| block_hasher=block_hasher, | ||
| ) |
There was a problem hiding this comment.
Similar to the other change in this PR, there is significant code duplication here when creating the Request object. This can be refactored to improve maintainability by extracting common arguments into a dictionary. The version check can also be made more concise and extensible.
request_kwargs = {
"request_id": f"id-{request_id}",
"prompt_token_ids": prompt_token_ids,
"sampling_params": sampling_params,
"pooling_params": [],
"eos_token_id": EOS_TOKEN_ID,
"block_hasher": block_hasher,
}
if any(vllm_version_is(v) for v in ["0.10.1.1", "0.10.1"]):
request_kwargs.update({
"multi_modal_kwargs": None,
"multi_modal_placeholders": None,
"multi_modal_hashes": None,
})
req = Request(**request_kwargs)Signed-off-by: weijinqian_v1 <weijinqian@huawei.com>
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (75.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2644 +/- ##
==========================================
- Coverage 72.61% 72.32% -0.29%
==========================================
Files 147 147
Lines 21805 21870 +65
==========================================
- Hits 15833 15818 -15
- Misses 5972 6052 +80
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…into main_829 * 'main_829' of https://github.com/raindaywhu/vllm-ascend: [torchair]remove aicpu op (vllm-project#2640) bugfix for torchair graph (vllm-project#2639) [CI] fix UT error. (vllm-project#2644) [3/N][Feat][Graph] Support `all-to-all` and quantized models with ACL Graph (vllm-project#2614) [Bugfix] Fix mc2 operator error in aclgraph + ep<16 scenario (vllm-project#2609)
vllm-project/vllm@69f4635 changed the vl input usage, this PR fix the related UT failure. - vLLM version: v0.10.1.1 - vLLM main: vllm-project/vllm@d660c98 --------- Signed-off-by: weijinqian_v1 <weijinqian@huawei.com> Co-authored-by: weijinqian_v1 <weijinqian@huawei.com> Signed-off-by: lijiaojiao <lijiaojiao990304@163.com>
vllm-project/vllm@69f4635 changed the vl input usage, this PR fix the related UT failure. - vLLM version: v0.10.1.1 - vLLM main: vllm-project/vllm@d660c98 --------- Signed-off-by: weijinqian_v1 <weijinqian@huawei.com> Co-authored-by: weijinqian_v1 <weijinqian@huawei.com>
vllm-project/vllm@69f4635 changed the vl input usage, this PR fix the related UT failure. - vLLM version: v0.10.1.1 - vLLM main: vllm-project/vllm@d660c98 --------- Signed-off-by: weijinqian_v1 <weijinqian@huawei.com> Co-authored-by: weijinqian_v1 <weijinqian@huawei.com>
vllm-project/vllm@69f4635 changed the vl input usage, this PR fix the related UT failure. - vLLM version: v0.10.1.1 - vLLM main: vllm-project/vllm@d660c98 --------- Signed-off-by: weijinqian_v1 <weijinqian@huawei.com> Co-authored-by: weijinqian_v1 <weijinqian@huawei.com>
2 participants
vllm-project/vllm@69f4635