Repository navigation
Conversation
Signed-off-by: HF-001 <1670186653@qq.com>
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 introduces support for speculative decoding using an external draft model within the Model Runner V2 (MRv2) architecture on Ascend devices. By implementing a custom speculator that adapts the upstream standalone AR speculator, the changes allow for efficient draft model execution while adhering to specific hardware constraints. The implementation includes necessary validation for model configurations, integration with Ascend-specific attention metadata builders, and robust test coverage to ensure parity between target and draft model outputs. 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
|
|
👋 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. Tip 💡 Consider Linking a Related Issue or RFCYour PR title contains the [Feature] tag, indicating a bug fix or new feature. Linking a related issue or RFC in the PR description is strongly encouraged — it gives reviewers helpful context and speeds up the review. You can use any of these keywords:
🙏 Thanks for helping us keep the project well-organized! |
There was a problem hiding this comment.
Code Review
Suggested PR Title:\n\nmarkdown\n[Attention][Feature] Support external draft models with Model Runner V2 on Ascend\n\n\nSuggested PR Summary:\n\nmarkdown\n### What this PR does / why we need it?\nThis PR introduces support for external draft models with Model Runner V2 (MRv2) on Ascend, targeting the upstream `standalone_ar.StandaloneARSpeculator` API. It implements `AscendStandaloneARSpeculator` to handle independent draft LM execution with exact FIA lengths and upstream eager drafting.\n\nFeedback:\n- In `_prefill`, explicitly cast both `input_batch.seq_lens` and `num_rejected` to `int32` before stacking to prevent potential dtype mismatch errors on Ascend NPUs.\n- In `_build_attn_metadata`, safely retrieve `_target_seq_lens_cpu` and `_num_rejected_cpu` using `getattr` to prevent `AttributeError` when `_prefill` is skipped or called with `skip_attn=True`.\n\n### Does this PR introduce _any_ user-facing change?\nYes, users can now enable external draft models with Model Runner V2 on Ascend by setting `VLLM_USE_V2_MODEL_RUNNER=1` and configuring `speculative_config` with `method="draft_model"`.\n\n### How was this patch tested?\n- Added unit tests in `tests/ut/worker/v2/test_standalone_ar_speculator.py` and `tests/ut/worker/v2/test_spec_decode.py`.\n- Added end-to-end tests in `tests/e2e/pull_request/one_card/model_runner_v2/test_qwen3_draft_model.py`.\n
| # FIA consumes CPU length lists. Copy once per proposal, not once | ||
| # per draft step. The target CPU mirror may be an upper bound under | ||
| # async scheduling, so derive exact lengths from the device buffers. | ||
| lengths = torch.stack((input_batch.seq_lens[:num_reqs], num_rejected[:num_reqs].int())).cpu() |
There was a problem hiding this comment.
To prevent potential dtype mismatch errors or unnecessary type conversions on Ascend NPUs, both input_batch.seq_lens and num_rejected should be explicitly cast to int32 (using .int()) before being stacked. On Ascend, int32 is the preferred integer type for attention metadata and indexing operations.
| lengths = torch.stack((input_batch.seq_lens[:num_reqs], num_rejected[:num_reqs].int())).cpu() | |
| lengths = torch.stack((input_batch.seq_lens[:num_reqs].int(), num_rejected[:num_reqs].int())).cpu() |
| if step == 0: | ||
| # Expanded prefill includes rejected placeholders; causal attention | ||
| # aligns queries using target length + one correction slot. | ||
| seq_lens_cpu = self._target_seq_lens_cpu + 1 | ||
| seq_lens_cpu_upper_bound = seq_lens_cpu | ||
| positions = self.expanded_positions | ||
| attn_state = AscendAttentionState.ChunkedPrefill | ||
| else: | ||
| # Upstream passes step + 1 to account for the correction token. | ||
| seq_lens_cpu = self._target_seq_lens_cpu - self._num_rejected_cpu + step | ||
| seq_lens_cpu_upper_bound = self._target_seq_lens_cpu - self._num_rejected_cpu | ||
| positions = self.input_buffers.positions | ||
| attn_state = AscendAttentionState.DecodeOnly |
There was a problem hiding this comment.
If _prefill is skipped or called with skip_attn=True (e.g., during profiling, dry-runs, or early initialization), self._target_seq_lens_cpu and self._num_rejected_cpu will not be initialized. This will cause an AttributeError when _build_attn_metadata is called. To make this robust and prevent crashes, retrieve these attributes safely using getattr and fallback to seq_lens_cpu_upper_bound and a zero tensor respectively if they are not set.
target_seq_lens_cpu = getattr(self, "_target_seq_lens_cpu", None)
num_rejected_cpu = getattr(self, "_num_rejected_cpu", None)
if target_seq_lens_cpu is None:
target_seq_lens_cpu = seq_lens_cpu_upper_bound
num_rejected_cpu = torch.zeros_like(target_seq_lens_cpu)
if step == 0:
# Expanded prefill includes rejected placeholders; causal attention
# aligns queries using target length + one correction slot.
seq_lens_cpu = target_seq_lens_cpu + 1
seq_lens_cpu_upper_bound = seq_lens_cpu
positions = self.expanded_positions
attn_state = AscendAttentionState.ChunkedPrefill
else:
# Upstream passes step + 1 to account for the correction token.
seq_lens_cpu = target_seq_lens_cpu - num_rejected_cpu + step
seq_lens_cpu_upper_bound = target_seq_lens_cpu - num_rejected_cpu
positions = self.input_buffers.positions
attn_state = AscendAttentionState.DecodeOnly73b49fd to
9251601
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
What this PR does / why we need it?
refer to : vllm-project/vllm#43091
How was this patch tested?