[Refactor] Clean up maybe_padded_num_tokens#5971
[Refactor] Clean up maybe_padded_num_tokens#5971gcanlin wants to merge 1 commit intovllm-project:mainfrom
Conversation
Signed-off-by: gcanlin <canlinguosdu@gmail.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 is a clean refactoring that removes the maybe_padded_num_tokens variable, which is no longer needed after the removal of torchair. The changes are consistent across the modified file, including updates to function signatures, return types, and variable usage. The refactoring is well-contained and improves code clarity by removing obsolete logic. I have reviewed the changes and they look good to be correct and safe. No issues were found.
|
cc @yiz-liu |
gcanlin
changed the title
| # Otherwise, it's just max_tokens_across_dp_cpu | ||
| (maybe_padded_num_tokens, num_tokens_across_dp, | ||
| (num_input_tokens, num_tokens_across_dp, | ||
| with_prefill) = self._sync_metadata_across_dp(num_input_tokens, |
There was a problem hiding this comment.
if maybe_padded_num_tokens is useless, why not update _sync_metadata_across_dp as well?
There was a problem hiding this comment.
IIUC, _sync_metadata_across_dp doesn't have additional logic to handle maybe_padded_num_tokens. Could you give some guidances for it?
There was a problem hiding this comment.
maybe_padded_num_tokens is added for NPUTorchairModelRunner, which overrides _sync_metadata_across_dp and returns maybe_padded_num_tokens. But after some torchair related logic had been removed, maybe_padded_num_tokens is totally same as num_input_tokens for now. So we still need to make num_input_tokens to accept the return value of _sync_metadata_across_dp.
Reference:
[1] #2582
[2] https://github.com/yiz-liu/vllm-ascend/blob/005b9af81676cb7b103d51bee3840861d3e70c96/vllm_ascend/torchair/torchair_model_runner.py#L73
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
2 participants
What this PR does / why we need it?
After torchair's removal, we don't need maybe_padded_num_tokens anymore. This PR cleans up it and didn't introduce other changes.
Does this PR introduce any user-facing change?
NO.
How was this patch tested?
Will be tested by CI.