[Test][310P] Cover unsupported sampler parameters - #15328
UniversePeak wants to merge 1 commit into
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 enhances the test suite for the Ascend310PSampler within the 310P model runner. The primary goal is to formalize the rejection behavior for unsupported sampling parameters, ensuring that future development does not inadvertently alter these constraints. The changes are strictly limited to testing infrastructure and do not affect the runtime behavior of the model. 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. |
![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 adds unit test coverage for the rejection of unsupported sampling parameters in Ascend310PSampler by introducing a parametrized test for penalty parameters and updating greedy/random sampling tests. The feedback recommends consolidating and expanding these tests into a single, comprehensive parametrized test to cover all unsupported parameters (such as top_p, top_k, min_p, logprobs, and logit_bias) that are currently missing from the test suite.
Suggested PR Title:
[Test][Misc] Add unit tests for unsupported sampling parameters in Ascend310PSamplerSuggested PR Summary:
### What this PR does / why we need it?
This PR adds focused unit test coverage for the rejection paths of unsupported sampling parameters in `Ascend310PSampler`. It introduces parametrized tests for repetition, presence, and frequency penalties, and refactors existing tests for greedy and random sampling.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Tested using the newly added unit tests in `tests/ut/_310p/test_model_runner_v2_310p.py`.| @pytest.mark.parametrize( | ||
| ("sampling_kwargs", "message"), | ||
| [ | ||
| ({"repetition_penalty": 1.1}, "repetition_penalty"), | ||
| ({"presence_penalty": 0.1}, "presence/frequency penalty"), | ||
| ({"frequency_penalty": 0.1}, "presence/frequency penalty"), | ||
| ( | ||
| {"presence_penalty": 0.1, "frequency_penalty": 0.1}, | ||
| "presence/frequency penalty", | ||
| ), | ||
| ], | ||
| ) | ||
| def test_sampler_rejects_unsupported_penalty_parameters(sampling_kwargs, message) -> None: | ||
| sampler = Ascend310PSampler() | ||
| with pytest.raises(NotImplementedError, match=message): | ||
| sampler.add_request(0, 4, SamplingParams(temperature=0, **sampling_kwargs)) | ||
|
|
||
|
|
||
| def test_sampler_rejects_random_sampling_parameters() -> None: | ||
| sampler = Ascend310PSampler() | ||
| with pytest.raises(NotImplementedError, match="Unsupported sampling parameters"): | ||
| sampler.add_request(1, 4, SamplingParams(temperature=1)) | ||
| sampler.add_request(0, 4, SamplingParams(temperature=1)) |
There was a problem hiding this comment.
The PR description states that this PR adds focused unit coverage for each rejection path of unsupported sampling parameters. However, several unsupported parameters checked in Ascend310PSampler.add_request (such as top_p, top_k, min_p, logprobs, prompt_logprobs, and logit_bias) are not covered by the new tests.
We can consolidate and expand the test coverage by replacing both test_sampler_rejects_unsupported_penalty_parameters and test_sampler_rejects_random_sampling_parameters with a single, comprehensive parametrized test that covers all unsupported parameters.
@pytest.mark.parametrize(
("sampling_kwargs", "message"),
[
({"temperature": 1.0}, "temperature"),
({"top_p": 0.9}, "top_p"),
({"top_k": 5}, "top_k"),
({"min_p": 0.1}, "min_p"),
({"repetition_penalty": 1.1}, "repetition_penalty"),
({"presence_penalty": 0.1}, "presence/frequency penalty"),
({"frequency_penalty": 0.1}, "presence/frequency penalty"),
(
{"presence_penalty": 0.1, "frequency_penalty": 0.1},
"presence/frequency penalty",
),
({"logprobs": 1}, "logprobs"),
({"prompt_logprobs": 1}, "logprobs"),
({"logit_bias": {1: 1.0}}, "logits processors"),
],
)
def test_sampler_rejects_unsupported_parameters(sampling_kwargs, message) -> None:
sampler = Ascend310PSampler()
temp = sampling_kwargs.pop("temperature", 0)
with pytest.raises(NotImplementedError, match=message):
sampler.add_request(0, 4, SamplingParams(temperature=temp, **sampling_kwargs))
|
CI gate is currently failing because The Gemini review also requests coverage for the remaining unsupported sampler parameters ( |
fdab2e4 to
6e3c828
Compare
|
Added focused coverage for the remaining unsupported 310P sampling parameters requested in review: Verification:
The update is in commit |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Merged the latest |
9f796fc to
3c157cf
Compare
|
The CI failure on 3c157cf was a test bug, not a sampler gap: the pinned vLLM main commit normalizes Pushed ba37d4f to build the |
|
Update: on the new head ba37d4f, |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
86b62f5 to
af29bf8
Compare
Add CPU UT coverage for unsupported sampler penalties on 310P (test_model_runner_v2_310p.py), rebased cleanly on current main as a single commit to avoid rebase conflicts in CI. Signed-off-by: UniversePeak <UniversePeak@users.noreply.github.com>
af29bf8 to
bc52f8c
Compare
|
Conflicts are resolved: the branch is now rebased on the latest main as a single clean commit (bc52f8c), so the CI "Rebase on main snapshot" step no longer hits the conflict in Current status on the latest commit: pre-commit ✅, DCO ✅, cpu-ut ✅. The ci-gate check reports this PR touches source code, so precision/full tests are required before merge — could a maintainer add the |
1 participant
What this PR does / why we need it?
The Ascend310PSampler for 310P model runner v2 intentionally rejects unsupported sampling parameters, including repetition, presence, and frequency penalties. This PR adds focused unit coverage for each rejection path so future changes cannot silently broaden the supported parameter set.
The existing greedy and random-sampling checks remain covered, and the new cases exercise the three penalty fields independently plus the combined presence/frequency path.
Related: #15235
Does this PR introduce any user-facing change?
No. This change only adds unit tests for existing rejection behavior.
How was this patch tested?
python -m py_compile tests/ut/_310p/test_model_runner_v2_310p.py vllm_ascend/_310p/worker/v2/sampler.pyruff check tests/ut/_310p/test_model_runner_v2_310p.pyruff format --check tests/ut/_310p/test_model_runner_v2_310p.pygit diff --checkThe focused pytest invocation was attempted, but this host's installed vLLM/NPU environment did not complete test collection within 300 seconds. The test file and sampler module compile successfully; full pytest and 310P hardware validation should run in CI or on maintainer hardware.
Signed-off-by: UniversePeak UniversePeak@users.noreply.github.com