test(sampling_params): repair broken test collection and add verify() coverage#1350
Conversation
The test module failed to collect because it imported and tested DecodeNode, which no longer exists after the nccl pd-mode removal. Drop the dead import and its test, remove the stale move_kv_to_decode_node kwarg, and add coverage for SamplingParams.verify(), which previously had none: the n == best_of constraint, best_of/n range limits, penalty and temperature/top_p/top_k bounds, min/max_new_tokens, and constraint mutual-exclusion.
![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 removes the DecodeNode test and its associated fields, and introduces a comprehensive suite of unit tests for verifying SamplingParams configurations using a new helper function _make_params. The review feedback correctly points out that since _make_params internally triggers verification during initialization, calling .verify() in tests that expect a ValueError is redundant and should be removed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def test_verify_rejects_n_not_equal_best_of(): | ||
| # The engine currently only supports n == best_of; a mismatch must be rejected. | ||
| with pytest.raises(ValueError): | ||
| _make_params(best_of=2, n=1).verify() |
There was a problem hiding this comment.
Since _make_params internally calls params.init(), which automatically runs self.verify(), any invalid parameter configuration will raise a ValueError during initialization. Consequently, the trailing .verify() call is never reached and is dead code. Removing .verify() makes the test cleaner and accurately reflects where the exception is raised.
| _make_params(best_of=2, n=1).verify() | |
| _make_params(best_of=2, n=1) |
| @pytest.mark.parametrize("best_of", [0, -1, MAX_BEST_OF + 1]) | ||
| def test_verify_rejects_best_of_out_of_range(best_of): | ||
| with pytest.raises(ValueError): | ||
| _make_params(best_of=best_of, n=best_of).verify() |
There was a problem hiding this comment.
Since _make_params internally calls params.init(), which automatically runs self.verify(), any invalid parameter configuration will raise a ValueError during initialization. Consequently, the trailing .verify() call is never reached and is dead code. Removing .verify() makes the test cleaner and accurately reflects where the exception is raised.
| _make_params(best_of=best_of, n=best_of).verify() | |
| _make_params(best_of=best_of, n=best_of) |
| ) | ||
| def test_verify_rejects_invalid_sampling_fields(field, value): | ||
| with pytest.raises(ValueError): | ||
| _make_params(**{field: value}).verify() |
There was a problem hiding this comment.
Since _make_params internally calls params.init(), which automatically runs self.verify(), any invalid parameter configuration will raise a ValueError during initialization. Consequently, the trailing .verify() call is never reached and is dead code. Removing .verify() makes the test cleaner and accurately reflects where the exception is raised.
| _make_params(**{field: value}).verify() | |
| _make_params(**{field: value}) |
|
|
||
| def test_verify_rejects_min_new_tokens_greater_than_max(): | ||
| with pytest.raises(ValueError): | ||
| _make_params(min_new_tokens=8, max_new_tokens=4).verify() |
There was a problem hiding this comment.
Since _make_params internally calls params.init(), which automatically runs self.verify(), any invalid parameter configuration will raise a ValueError during initialization. Consequently, the trailing .verify() call is never reached and is dead code. Removing .verify() makes the test cleaner and accurately reflects where the exception is raised.
| _make_params(min_new_tokens=8, max_new_tokens=4).verify() | |
| _make_params(min_new_tokens=8, max_new_tokens=4) |
| def test_verify_rejects_regular_constraint_with_allowed_token_ids(): | ||
| # regular_constraint and allowed_token_ids are mutually exclusive. | ||
| with pytest.raises(ValueError): | ||
| _make_params(regular_constraint="[a-z]+", allowed_token_ids=[1, 2, 3]).verify() |
There was a problem hiding this comment.
Since _make_params internally calls params.init(), which automatically runs self.verify(), any invalid parameter configuration will raise a ValueError during initialization. Consequently, the trailing .verify() call is never reached and is dead code. Removing .verify() makes the test cleaner and accurately reflects where the exception is raised.
| _make_params(regular_constraint="[a-z]+", allowed_token_ids=[1, 2, 3]).verify() | |
| _make_params(regular_constraint="[a-z]+", allowed_token_ids=[1, 2, 3]) |
1 participant
What
unit_tests/server/core/objs/test_sampling_params.pycurrently fails at collection:DecodeNode(and themove_kv_to_decode_nodesampling field) were removed together with the nccl PD mode, so the module-level import on line 8 raisesImportErrorand pytest cannot collect any test in the file.This PR:
DecodeNodeimport andtest_decode_node_initialization, and drops the stalemove_kv_to_decode_nodekwarg fromtest_sampling_params_initialization, so the module collects again.SamplingParams.verify(), which previously had none even though it is the per-request input-validation gate. The new tests cover:n == best_ofconstraint andbest_of/nrange limits (MAX_BEST_OF);presence_penalty/frequency_penalty/repetition_penaltybounds;temperature,top_p,top_kbounds (and validtop_kvalues);max_new_tokens/min_new_tokensbounds and their ordering;regular_constraintandallowed_token_ids.A small
_make_params()helper builds a valid configuration and applies per-test overrides. It setsdo_sample=Trueso the sampling fields are validated as provided (greedy decoding resets them insideinit).Why
The broken import silently disabled the entire sampling-params test module, and
verify()had no direct tests. Restoring collection and adding these cases guards the request-validation rules against regressions.Test
Formatting verified with the project's
pre-commithooks (black--line-length=120, flake8).