-
Notifications
You must be signed in to change notification settings - Fork 386
[Misc] Refacotr prepare_reqs #2335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the execute_model
method to improve readability by splitting the large _process_reqs
method into _prepare_inputs
and _execute_model
. This is a good change that makes the code easier to follow. The logic seems to be preserved correctly after the refactoring. I've found one issue with an outdated type hint that should be addressed. Also, there is a small typo in the pull request title ('Refacotr' should be 'Refactor').
) -> tuple[Union[AscendMetadata, AscendMLAMetadata, | ||
AscendTorchairMetadata], torch.Tensor, SpecDecodeMetadata, | ||
torch.Tensor, int, torch.Tensor, torch.Tensor, np.ndarray, | ||
Optional[set[str]], Optional[set[str]]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return type hint for this function is outdated after the refactoring and no longer matches the actual returned values. Please update it to reflect the new return signature.
) -> tuple[Union[AscendMetadata, AscendMLAMetadata, | |
AscendTorchairMetadata], torch.Tensor, SpecDecodeMetadata, | |
torch.Tensor, int, torch.Tensor, torch.Tensor, np.ndarray, | |
Optional[set[str]], Optional[set[str]]]: | |
) -> tuple[Union[AscendMetadata, AscendMLAMetadata, | |
AscendTorchairMetadata], torch.Tensor, np.ndarray, int, | |
Optional[torch.Tensor], bool, Optional[torch.Tensor], | |
Optional[torch.Tensor], np.ndarray]: |
👋 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. |
Signed-off-by: wangxiyuan <[email protected]>
ca6f1c2
to
5fcb122
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2335 +/- ##
=======================================
Coverage 76.35% 76.35%
=======================================
Files 117 117
Lines 13371 13371
=======================================
Hits 10209 10209
Misses 3162 3162
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:
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Before do more refactor, we should make
execute_model
more readable.