Repository navigation
Simplify test_chunked_prefill; remove redundant tests - #22652
Conversation
There was a problem hiding this comment.
Code Review
This pull request simplifies the chunked prefill test suite by removing several test cases, including those for multi-request scenarios. Feedback indicates that the remaining tests should explicitly define "chunked_prefill_size" to ensure the chunked prefill logic is properly exercised. Furthermore, it is recommended to restore and adjust the multi-request test case rather than removing it, as it provides critical coverage for concurrent request scheduling.
I am having trouble creating individual review comments. Click here to see my feedback.
test/registered/scheduler/test_chunked_prefill.py (16)
The test test_mixed_chunked_prefill does not explicitly set chunked_prefill_size. In the scheduler implementation, is_mixed_chunk is only enabled if chunked_prefill_size is not None. Without this parameter, the test may not be exercising the chunked prefill logic. Consider explicitly passing a chunked_prefill_size (e.g., 128) to ensure the intended code path is tested.
run_mmlu_test(disable_radix_cache=False, enable_mixed_chunk=True, chunked_prefill_size=128)
test/registered/scheduler/test_chunked_prefill.py (19)
Similar to the test above, chunked_prefill_size should be explicitly set here to ensure that chunked prefill is actually active during the test.
run_mmlu_test(disable_radix_cache=True, enable_mixed_chunk=True, chunked_prefill_size=128)
test/registered/scheduler/test_chunked_prefill.py (27-31)
Removing test_mixed_chunked_prefill_multi_requests eliminates coverage for concurrent requests with chunked prefill. While the current implementation might not trigger chunking frequently with a size of 2048 on short prompts, it is better to fix the test by reducing the chunked_prefill_size (e.g., to 128) rather than removing it. This provides valuable coverage for the scheduler's state management under load, which sequential MMLU tests may miss.
|
/rerun-test test_mixed_chunked_prefill.py |
|
✅ |
|
/tag-and-rerun-ci |
Summary
Remove redundant tests in
test_chunked_prefill.pyto reduce CI time:test_chunked_prefill— default path (radix_cache=True, mixed_chunk=False), implicitly covered by all other MMLU integration teststest_chunked_prefill_without_radix_cache— disable_radix_cache path is already covered bytest_no_overlap_scheduler.pytest_mixed_chunked_prefill_multi_requests— no accuracy assertion (justresponse.json()); chunked_prefill_size=2048 rarely triggers chunking on short promptsKeep:
test_mixed_chunked_prefill— unique coverage ofenable_mixed_chunk=Trueon default pathtest_mixed_chunked_prefill_without_radix_cache— unique coverage ofmixed_chunk + disable_radix_cachecomboTest plan
test_chunked_prefill.pypasses with remaining 2 testsest_time(currently 360) after CI run