-
Notifications
You must be signed in to change notification settings - Fork 604
[wip]recompute scheduler adapt main #4460
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: CHEN <[email protected]>
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 is a large-scale adaptation of the RecomputeScheduler to align with recent changes in the main vLLM scheduler. It includes significant refactoring, such as renaming the class, updating its inheritance, and modifying many core methods like schedule and update_from_output. The changes introduce support for new features like hybrid memory allocation (HMA), encoder cache transfer (ECConnector), and the v2 model runner. It also brings substantial improvements in robustness, particularly in handling distributed KV cache transfers, preemption logic, and error recovery from invalid cache blocks.
My review has identified two critical issues that need to be addressed. First, the scheduler class was renamed in a way that will break its loading via configuration. Second, a dataclass definition contains a type mismatch and a risky default value, which could lead to runtime errors. After these issues are fixed, this PR will be a great step forward in keeping the Ascend-specific scheduler up-to-date with the core vLLM project.
| """This Scheduler extends vllm's original v1 scheduler of version 0.11 | ||
| to fix recomputing bug.""" | ||
|
|
||
| class Scheduler(SchedulerInterface): |
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 class has been renamed from RecomputeScheduler to Scheduler. However, the configuration file vllm_ascend/core/recompute_schedule_config.py still refers to vllm_ascend.core.recompute_scheduler.RecomputeScheduler. This will cause an ImportError when vLLM tries to load the scheduler class at runtime. To fix this, the class should be renamed back to RecomputeScheduler to match the name used in the configuration.
| class Scheduler(SchedulerInterface): | |
| class RecomputeScheduler(SchedulerInterface): |
| class RecomputeSchedulerOutput(SchedulerOutput): | ||
| # requests that need to recompute kv | ||
| recomputed_reqs: list[RecomputeReqInfo] | ||
|
|
||
| # KV Cache Connector metadata. | ||
| kv_connector_metadata: Optional[KVConnectorMetadata] = None | ||
| recomputed_reqs: list[RecomputeReqInfo] = None |
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 recomputed_reqs field is defined with a type hint of list[RecomputeReqInfo] but has a default value of None. This is a type mismatch and can lead to a TypeError at runtime, for instance in update_from_output where this field is iterated over without a None check. The default value should be an empty list to match the type hint and ensure safe iteration.
| class RecomputeSchedulerOutput(SchedulerOutput): | |
| # requests that need to recompute kv | |
| recomputed_reqs: list[RecomputeReqInfo] | |
| # KV Cache Connector metadata. | |
| kv_connector_metadata: Optional[KVConnectorMetadata] = None | |
| recomputed_reqs: list[RecomputeReqInfo] = None | |
| class RecomputeSchedulerOutput(SchedulerOutput): | |
| # requests that need to recompute kv | |
| recomputed_reqs: list[RecomputeReqInfo] = [] |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
👋 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?
recompute scheduler adapt main
Does this PR introduce any user-facing change?
How was this patch tested?