[Misc]Fix AttributeError in RecomputeScheduler due to missing routed experts extraction logic - #10275
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an AttributeError in the RecomputeScheduler by introducing a more comprehensive mechanism for extracting and managing routed experts. It ensures that routing data is correctly persisted and retrieved, accounting for different vLLM version requirements and specific request states such as prompt processing versus token generation. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
Suggested PR Title:
[Ops][Feature] Support returning routed experts in RecomputeSchedulerSuggested PR Summary:
### What this PR does / why we need it?
This pull request adds support for retrieving and persisting routed experts (MoE routing data) in the `RecomputeScheduler` for Ascend. It introduces version-specific handling for vLLM (supporting both version "0.21.0" and others) to store batch routing data and map it back to individual requests.
Feedback:
- A critical issue was identified where `self._re_block_ids` is never populated in `RecomputeScheduler`, causing `self._re_block_ids.pop(req_id, [])` to always return an empty list. This should be replaced with `self.kv_cache_manager.get_block_ids(req_id)`.
- A potential `KeyError` could occur when accessing `num_scheduled_tokens[rid]`. It is safer to use `num_scheduled_tokens.get(rid, 0)`.
### Does this PR introduce _any_ user-facing change?
No, this is an internal update to the scheduler.
### How was this patch tested?
No tests were provided in this PR.| elif getattr(self, "enable_return_routed_experts", False) and routing_data is not None and new_token_ids: | ||
| req_offset = routing_offsets[req_id] | ||
| end = req_offset + num_tokens_scheduled | ||
| block_ids = self._re_block_ids.pop(req_id, []) |
There was a problem hiding this comment.
The self._re_block_ids dictionary is never populated in RecomputeScheduler.schedule (which completely overrides the parent Scheduler.schedule method). As a result, self._re_block_ids.pop(req_id, []) will always return an empty list [], causing self.routed_experts_mgr.get to be called with no block IDs and fail to retrieve the routed experts. To fix this, retrieve the block IDs directly from the kv_cache_manager using self.kv_cache_manager.get_block_ids(req_id).
| block_ids = self._re_block_ids.pop(req_id, []) | |
| block_ids = self.kv_cache_manager.get_block_ids(req_id) |
| offset = 0 | ||
| for rid in model_runner_output.req_ids: | ||
| routing_offsets[rid] = offset | ||
| offset += num_scheduled_tokens[rid] |
There was a problem hiding this comment.
If a request ID in model_runner_output.req_ids is missing from num_scheduled_tokens due to any discrepancy, accessing num_scheduled_tokens[rid] directly will raise a KeyError and crash the engine. Using .get(rid, 0) is safer and prevents potential crashes.
| offset += num_scheduled_tokens[rid] | |
| offset += num_scheduled_tokens.get(rid, 0) |
fcbbc31 to
f0e4714
Compare
|
👋 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. |
f0e4714 to
fa7b8ad
Compare
…experts extraction logic (vllm-project#10275) ### What this PR does / why we need it? part of vllm-project#10215 Summary Fixes a crash in RecomputeScheduler / AsyncRecomputeScheduler when a request finishes, caused by calling a non-existent _get_routed_experts() method: AttributeError: 'AsyncRecomputeScheduler' object has no attribute '_get_routed_experts' This is triggered when all of the following are true: Async scheduling is enabled (uses AsyncRecomputeScheduler) A request stops in the current step (stopped=True) Reproducible on both vLLM v0.21.0 and main commit 9090368. ### Does this PR introduce _any_ user-facing change? Yes. After this fix, in async scheduling + recompute scheduler scenarios: The service no longer crashes due to the missing _get_routed_experts method When enable_return_routed_experts=True, routed experts are correctly returned via EngineCoreOutput No API or interface changes. ### How was this patch tested? - vLLM version: v0.21.0 - vLLM main: vllm-project/vllm@9090368 --------- Signed-off-by: nofushanquan <1255959842@qq.com> Co-authored-by: nofushanquan <1255959842@qq.com> Signed-off-by: zhutianyi <1589841300@qq.com>
…experts extraction logic (vllm-project#10275) ### What this PR does / why we need it? part of vllm-project#10215 Summary Fixes a crash in RecomputeScheduler / AsyncRecomputeScheduler when a request finishes, caused by calling a non-existent _get_routed_experts() method: AttributeError: 'AsyncRecomputeScheduler' object has no attribute '_get_routed_experts' This is triggered when all of the following are true: Async scheduling is enabled (uses AsyncRecomputeScheduler) A request stops in the current step (stopped=True) Reproducible on both vLLM v0.21.0 and main commit 9090368. ### Does this PR introduce _any_ user-facing change? Yes. After this fix, in async scheduling + recompute scheduler scenarios: The service no longer crashes due to the missing _get_routed_experts method When enable_return_routed_experts=True, routed experts are correctly returned via EngineCoreOutput No API or interface changes. ### How was this patch tested? - vLLM version: v0.21.0 - vLLM main: vllm-project/vllm@9090368 --------- Signed-off-by: nofushanquan <1255959842@qq.com> Co-authored-by: nofushanquan <1255959842@qq.com>
…experts extraction logic (vllm-project#10275) ### What this PR does / why we need it? part of vllm-project#10215 Summary Fixes a crash in RecomputeScheduler / AsyncRecomputeScheduler when a request finishes, caused by calling a non-existent _get_routed_experts() method: AttributeError: 'AsyncRecomputeScheduler' object has no attribute '_get_routed_experts' This is triggered when all of the following are true: Async scheduling is enabled (uses AsyncRecomputeScheduler) A request stops in the current step (stopped=True) Reproducible on both vLLM v0.21.0 and main commit 9090368. ### Does this PR introduce _any_ user-facing change? Yes. After this fix, in async scheduling + recompute scheduler scenarios: The service no longer crashes due to the missing _get_routed_experts method When enable_return_routed_experts=True, routed experts are correctly returned via EngineCoreOutput No API or interface changes. ### How was this patch tested? - vLLM version: v0.21.0 - vLLM main: vllm-project/vllm@9090368 --------- Signed-off-by: nofushanquan <1255959842@qq.com> Co-authored-by: nofushanquan <1255959842@qq.com>
…experts extraction logic (vllm-project#10275) ### What this PR does / why we need it? part of vllm-project#10215 Summary Fixes a crash in RecomputeScheduler / AsyncRecomputeScheduler when a request finishes, caused by calling a non-existent _get_routed_experts() method: AttributeError: 'AsyncRecomputeScheduler' object has no attribute '_get_routed_experts' This is triggered when all of the following are true: Async scheduling is enabled (uses AsyncRecomputeScheduler) A request stops in the current step (stopped=True) Reproducible on both vLLM v0.21.0 and main commit 9090368. ### Does this PR introduce _any_ user-facing change? Yes. After this fix, in async scheduling + recompute scheduler scenarios: The service no longer crashes due to the missing _get_routed_experts method When enable_return_routed_experts=True, routed experts are correctly returned via EngineCoreOutput No API or interface changes. ### How was this patch tested? - vLLM version: v0.21.0 - vLLM main: vllm-project/vllm@9090368 --------- Signed-off-by: nofushanquan <1255959842@qq.com> Co-authored-by: nofushanquan <1255959842@qq.com>
…experts extraction logic (vllm-project#10275) ### What this PR does / why we need it? part of vllm-project#10215 Summary Fixes a crash in RecomputeScheduler / AsyncRecomputeScheduler when a request finishes, caused by calling a non-existent _get_routed_experts() method: AttributeError: 'AsyncRecomputeScheduler' object has no attribute '_get_routed_experts' This is triggered when all of the following are true: Async scheduling is enabled (uses AsyncRecomputeScheduler) A request stops in the current step (stopped=True) Reproducible on both vLLM v0.21.0 and main commit 9090368. ### Does this PR introduce _any_ user-facing change? Yes. After this fix, in async scheduling + recompute scheduler scenarios: The service no longer crashes due to the missing _get_routed_experts method When enable_return_routed_experts=True, routed experts are correctly returned via EngineCoreOutput No API or interface changes. ### How was this patch tested? - vLLM version: v0.21.0 - vLLM main: vllm-project/vllm@9090368 --------- Signed-off-by: nofushanquan <1255959842@qq.com> Co-authored-by: nofushanquan <1255959842@qq.com>
…experts extraction logic (vllm-project#10275) ### What this PR does / why we need it? part of vllm-project#10215 Summary Fixes a crash in RecomputeScheduler / AsyncRecomputeScheduler when a request finishes, caused by calling a non-existent _get_routed_experts() method: AttributeError: 'AsyncRecomputeScheduler' object has no attribute '_get_routed_experts' This is triggered when all of the following are true: Async scheduling is enabled (uses AsyncRecomputeScheduler) A request stops in the current step (stopped=True) Reproducible on both vLLM v0.21.0 and main commit 9090368. ### Does this PR introduce _any_ user-facing change? Yes. After this fix, in async scheduling + recompute scheduler scenarios: The service no longer crashes due to the missing _get_routed_experts method When enable_return_routed_experts=True, routed experts are correctly returned via EngineCoreOutput No API or interface changes. ### How was this patch tested? - vLLM version: v0.21.0 - vLLM main: vllm-project/vllm@9090368 --------- Signed-off-by: nofushanquan <1255959842@qq.com> Co-authored-by: nofushanquan <1255959842@qq.com>
…experts extraction logic (vllm-project#10275) ### What this PR does / why we need it? part of vllm-project#10215 Summary Fixes a crash in RecomputeScheduler / AsyncRecomputeScheduler when a request finishes, caused by calling a non-existent _get_routed_experts() method: AttributeError: 'AsyncRecomputeScheduler' object has no attribute '_get_routed_experts' This is triggered when all of the following are true: Async scheduling is enabled (uses AsyncRecomputeScheduler) A request stops in the current step (stopped=True) Reproducible on both vLLM v0.21.0 and main commit 9090368. ### Does this PR introduce _any_ user-facing change? Yes. After this fix, in async scheduling + recompute scheduler scenarios: The service no longer crashes due to the missing _get_routed_experts method When enable_return_routed_experts=True, routed experts are correctly returned via EngineCoreOutput No API or interface changes. ### How was this patch tested? - vLLM version: v0.21.0 - vLLM main: vllm-project/vllm@9090368 --------- Signed-off-by: nofushanquan <1255959842@qq.com> Co-authored-by: nofushanquan <1255959842@qq.com>
…experts extraction logic (vllm-project#10275) ### What this PR does / why we need it? part of vllm-project#10215 Summary Fixes a crash in RecomputeScheduler / AsyncRecomputeScheduler when a request finishes, caused by calling a non-existent _get_routed_experts() method: AttributeError: 'AsyncRecomputeScheduler' object has no attribute '_get_routed_experts' This is triggered when all of the following are true: Async scheduling is enabled (uses AsyncRecomputeScheduler) A request stops in the current step (stopped=True) Reproducible on both vLLM v0.21.0 and main commit 9090368. ### Does this PR introduce _any_ user-facing change? Yes. After this fix, in async scheduling + recompute scheduler scenarios: The service no longer crashes due to the missing _get_routed_experts method When enable_return_routed_experts=True, routed experts are correctly returned via EngineCoreOutput No API or interface changes. ### How was this patch tested? - vLLM version: v0.21.0 - vLLM main: vllm-project/vllm@9090368 --------- Signed-off-by: nofushanquan <1255959842@qq.com> Co-authored-by: nofushanquan <1255959842@qq.com>
…experts extraction logic (vllm-project#10275) ### What this PR does / why we need it? part of vllm-project#10215 Summary Fixes a crash in RecomputeScheduler / AsyncRecomputeScheduler when a request finishes, caused by calling a non-existent _get_routed_experts() method: AttributeError: 'AsyncRecomputeScheduler' object has no attribute '_get_routed_experts' This is triggered when all of the following are true: Async scheduling is enabled (uses AsyncRecomputeScheduler) A request stops in the current step (stopped=True) Reproducible on both vLLM v0.21.0 and main commit 9090368. ### Does this PR introduce _any_ user-facing change? Yes. After this fix, in async scheduling + recompute scheduler scenarios: The service no longer crashes due to the missing _get_routed_experts method When enable_return_routed_experts=True, routed experts are correctly returned via EngineCoreOutput No API or interface changes. ### How was this patch tested? - vLLM version: v0.21.0 - vLLM main: vllm-project/vllm@9090368 --------- Signed-off-by: nofushanquan <1255959842@qq.com> Co-authored-by: nofushanquan <1255959842@qq.com>
What this PR does / why we need it?
part of #10215
Summary
Fixes a crash in RecomputeScheduler / AsyncRecomputeScheduler when a request finishes, caused by calling a non-existent _get_routed_experts() method:
AttributeError: 'AsyncRecomputeScheduler' object has no attribute '_get_routed_experts'
This is triggered when all of the following are true:
Async scheduling is enabled (uses AsyncRecomputeScheduler)
A request stops in the current step (stopped=True)
Reproducible on both vLLM v0.21.0 and main commit 9090368.
Does this PR introduce any user-facing change?
Yes. After this fix, in async scheduling + recompute scheduler scenarios:
The service no longer crashes due to the missing _get_routed_experts method
When enable_return_routed_experts=True, routed experts are correctly returned via EngineCoreOutput
No API or interface changes.
How was this patch tested?