✅ CB tests refactoring + adding batch test #257

prashantgupta24 · 2025-06-23T19:59:58Z

Description

✅ change max_model_len to 256
✅ edit HF test description
✅add pytest.xfail for failing CB tests
✅ run batch handling test for CB too

Issues

Fix Add test for batch handling with requests finishing at different times #256

Signed-off-by: Prashant Gupta <[email protected]>

github-actions · 2025-06-23T20:00:10Z

👋 Hi! Thank you for contributing to vLLM support on Spyre.
Just a reminder: Make sure that your code passes all the linting checks, otherwise your PR won't be able to be merged. To do so, first install the linting requirements, then run format.sh and commit the changes. This can be done with uv directly:

uv sync --frozen --group lint --active --inexact

Or this can be done with pip:

uv pip compile --group lint > requirements-lint.txt
pip install -r requirements-lint.txt
bash format.sh

Now you are good to go 🚀

performance improvement Signed-off-by: Prashant Gupta <[email protected]>

Signed-off-by: Prashant Gupta <[email protected]>

tests/e2e/test_spyre_cb.py

Signed-off-by: Joe Runde <[email protected]>

yannicks1

lgtm in general, provided some comments which would be nice to be addressed before merging.

yannicks1 · 2025-06-25T08:10:51Z

tests/e2e/test_spyre_basic.py

@@ -52,6 +38,7 @@ def test_output(
    test using 'pytest --capture=no tests/spyre/test_spyre_basic.py'
    After debugging, DISABLE_ASSERTS should be reset to 'False'.
    '''
+    prompts = get_chicken_soup_prompts(4)


good idea to not have prompts as parameters!

yannicks1 · 2025-06-25T08:38:17Z

tests/e2e/test_spyre_basic.py

        sampling_params=vllm_sampling_params,
        tensor_parallel_size=1,
        backend=backend,
-        monkeypatch=monkeypatch)
+        max_num_seqs=2,


could this be packed into kwargs only in case cb == 1?

yannicks1 · 2025-06-25T08:38:52Z

tests/e2e/test_spyre_basic.py

-    prompts = [
-        "7 6 5 4",
-        "10 9 8 7",
-        "8 7 6 5",
-        "10 9 8 7 ",


nice to switch to comparing to hf outputs here too!

yannicks1 · 2025-06-25T08:49:21Z

tests/e2e/test_spyre_cb.py

-    template.format("Convert char to string in Java."),
-]])
-def test_cb_handling(
+@pytest.mark.parametrize("max_num_seqs", [2, 4],


what is the motivation of removing max_num_seqs 3 here? it will be expected to fail with xfail anyway, right?

Just reducing the total number of tests running, since these all take quite a while to run and I didn't think there was anything specific about max_num_seqs=3 that we needed to test.

Do you think there's a good chance we'll miss a bug if we don't run with 3?

sounds good!

yannicks1 · 2025-06-25T08:56:02Z

tests/e2e/test_spyre_cb.py

@@ -85,20 +72,18 @@ def test_cb_handling(


 @pytest.mark.cb
-@pytest.mark.parametrize("max_num_seqs", [2])


This could also stay here as we want to support batch size > 2 soon (and test it here). Could use xfail similar in the above test_cb_output.

Or is it not required to also test for batch size > 2 for test_cb_max_tokens?

Or is it not required to also test for batch size > 2 for test_cb_max_tokens?

Right, we're testing that the prompts are rejected before even running the model so I don' think it's relevant to parameterize this on max batch size.

yannicks1 · 2025-06-25T08:58:08Z

tests/e2e/test_spyre_cb.py

@@ -643,7 +628,6 @@ def augment_checked_steps(
 @pytest.mark.cb
 @pytest.mark.parametrize("model", get_spyre_model_list())
 @pytest.mark.parametrize("backend", get_spyre_backend_list())
-@pytest.mark.parametrize("max_num_seqs", [2])


same question (see above)

Yeah, my understanding here is that the get_params_test_* methods are all already assuming max batch size 2, so this can't be parameterized higher right now

yannicks1 · 2025-06-25T09:09:31Z

tests/e2e/test_spyre_cb.py

@@ -643,7 +628,6 @@ def augment_checked_steps(
 @pytest.mark.cb
 @pytest.mark.parametrize("model", get_spyre_model_list())
 @pytest.mark.parametrize("backend", get_spyre_backend_list())
-@pytest.mark.parametrize("max_num_seqs", [2])
 @pytest.mark.parametrize(
    "seqs_max_tokens,prompts_lengths,steps_add_reqs,checked_steps,"
    "max_model_len", [


(I know this is not your change, but) could we move max_model_len out of the get_params* functions as part of this refactoring PR? It is not test case specific and should not be needed to set at 5 different places (currently it is set to 256 in all functions)

Signed-off-by: Joe Runde <[email protected]>

joerunde · 2025-06-25T16:10:23Z

mergin'!

prashantgupta24 added 2 commits June 23, 2025 12:55

🎨 change max_model_len to 256

5d8435f

Signed-off-by: Prashant Gupta <[email protected]>

🎨 edit HF test description

2d1db9c

Signed-off-by: Prashant Gupta <[email protected]>

prashantgupta24 added 6 commits June 23, 2025 13:00

🎨 moving max_num_seqs down as a

ca48048

performance improvement Signed-off-by: Prashant Gupta <[email protected]>

🎨 add some xfails

7bc9272

Signed-off-by: Prashant Gupta <[email protected]>

✅ add test for batch handling

44b566d

Signed-off-by: Prashant Gupta <[email protected]>

🎨 fmt

29b9c84

Signed-off-by: Prashant Gupta <[email protected]>

🐛 fix typo

9789565

Signed-off-by: Prashant Gupta <[email protected]>

🎨 shifting xfail to test function

a36c57c

Signed-off-by: Prashant Gupta <[email protected]>

prashantgupta24 changed the title ~~wip Cb tests refactor~~ ✅ CB tests refactoring + adding batch test Jun 23, 2025

prashantgupta24 commented Jun 23, 2025

View reviewed changes

tests/e2e/test_spyre_cb.py Outdated Show resolved Hide resolved

joerunde added 2 commits June 24, 2025 16:10

🐛 xfail properly

fa9d819

Signed-off-by: Joe Runde <[email protected]>

♻️ combine sb and cb batch handling tests

1f3fd01

Signed-off-by: Joe Runde <[email protected]>

joerunde marked this pull request as ready for review June 24, 2025 23:03

joerunde requested review from rafvasq and sducouedic as code owners June 24, 2025 23:03

yannicks1 approved these changes Jun 25, 2025

View reviewed changes

🎨 update tests from review comments

8bbb0fd

Signed-off-by: Joe Runde <[email protected]>

joerunde merged commit e442585 into main Jun 25, 2025
17 checks passed

joerunde deleted the cb-tests-refactor branch June 25, 2025 16:10

		@@ -85,20 +72,18 @@ def test_cb_handling(


		@pytest.mark.cb
		@pytest.mark.parametrize("max_num_seqs", [2])

✅ CB tests refactoring + adding batch test #257

✅ CB tests refactoring + adding batch test #257

Uh oh!

Conversation

prashantgupta24 commented Jun 23, 2025 • edited by joerunde Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues

Uh oh!

github-actions bot commented Jun 23, 2025

Uh oh!

Uh oh!

yannicks1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joerunde commented Jun 25, 2025

Uh oh!

Uh oh!

Uh oh!

prashantgupta24 commented Jun 23, 2025 •

edited by joerunde

Loading