Skip to content

[CB][Tests] Check output of scheduling tests on Spyre #337

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 31 commits into from
Jul 30, 2025
Merged

[CB][Tests] Check output of scheduling tests on Spyre #337

merged 31 commits into from
Jul 30, 2025

Conversation

sducouedic
Copy link
Collaborator

@sducouedic sducouedic commented Jul 25, 2025 •
edited
Loading

This code adds some logic to check the ouput of the scheduling steps tests. The output checking is done only on Spyre to save some compute for the cpu tests.

[Important note]
The output comparison checking of scheduling tests don't pass on CPU neither do they on Spyre. Exact reasons why are not sure, on cpu it might be because of high entropy of the randomly generated prompt tokens.

sducouedic added 12 commits July 21, 2025 13:17
@sducouedic
save as I have issue in my container
a5d7fa1 
Signed-off-by: Sophie du Couédic <[email protected]>
@sducouedic
save
84db72b 
Signed-off-by: Sophie du Couédic <[email protected]>
@sducouedic
finished test
608ba62 
Signed-off-by: Sophie du Couédic <[email protected]>
@sducouedic
add 'max_model_len' as parameter
6e1f33a 
Signed-off-by: Sophie du Couédic <[email protected]>
@sducouedic
collect output
0868f1e 
Signed-off-by: Sophie du Couédic <[email protected]>
@sducouedic
Revert "collect output"
843e7ca 
This reverts commit 0868f1e.

Signed-off-by: Sophie du Couédic <[email protected]>
@sducouedic
decode 64 instead of 65 for last prompt
a503ef1 
Signed-off-by: Sophie du Couédic <[email protected]>
@sducouedic
Reapply "collect output"
6b5d0fd 
This reverts commit 843e7ca.

Signed-off-by: Sophie du Couédic <[email protected]>
@sducouedic
format.sh
59e7270 
Signed-off-by: Sophie du Couédic <[email protected]>
@sducouedic
'check_scheduler_inference_steps' create random prompts from vocal an…
96e3175 
…d returns it

Signed-off-by: Sophie du Couédic <[email protected]>
@sducouedic
compare results naive
ac373c0 
Signed-off-by: Sophie du Couédic <[email protected]>
@sducouedic
remove joins test
46cb6ab 
Signed-off-by: Sophie du Couédic <[email protected]>
@github-actions GitHub Actions
Copy link

👋 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 🚀

@sducouedic sducouedic force-pushed the scheduling_tests_check_output branch from 877ac8e to 44b630d Compare July 25, 2025 14:34
@sducouedic
merge with main
71315bd 
Signed-off-by: Sophie du Couédic <[email protected]>
@sducouedic sducouedic force-pushed the scheduling_tests_check_output branch from 44b630d to 71315bd Compare July 25, 2025 15:06
@sducouedic sducouedic marked this pull request as ready for review July 25, 2025 23:34
prashantgupta24
prashantgupta24 reviewed Jul 28, 2025
View reviewed changes
maxdebayser
maxdebayser reviewed Jul 29, 2025
View reviewed changes
tests/conftest.py


@pytest.fixture
def set_random_seed(request):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a type hint for the request parameter?

maxdebayser
maxdebayser approved these changes Jul 29, 2025
View reviewed changes
Copy link
Collaborator

@maxdebayser maxdebayser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the other reviews that the compare with HF block can be factored out into a function. Other than that, this PR LGTM.

prashantgupta24
prashantgupta24 requested changes Jul 29, 2025
View reviewed changes
Copy link
Collaborator

@prashantgupta24 prashantgupta24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The output comparison checking of scheduling tests don't pass on CPU neither do they on Spyre. Exact reasons why are not sure, on cpu it might be because of high entropy of the randomly generated prompt tokens.

That's concerning. Blocking this PR until we get to the bottom of that

@prashantgupta24
🎨 add line at end
2090829 
Signed-off-by: Prashant Gupta <[email protected]>
@prashantgupta24
♻️ no need for else after return
b33db5b 
Signed-off-by: Prashant Gupta <[email protected]>
@prashantgupta24
Copy link
Collaborator

I'm also not sure if we want to go down the route of comparing outputs against transformers or do we want to switch to comparing outputs against static batching?

@prashantgupta24
🎨 fmt
c58bb6c 
Signed-off-by: Prashant Gupta <[email protected]>
@prashantgupta24
Merge remote-tracking branch 'upstream/main' into scheduling_tests_ch…
c35f06a 
…eck_output
@prashantgupta24
Merge remote-tracking branch 'upstream/main' into scheduling_tests_ch…
8424a63 
…eck_output
@prashantgupta24
♻️ make output checking default
5636eb1 
Signed-off-by: Prashant Gupta <[email protected]>
@prashantgupta24
♻️ use check_output_against_hf everywhere
3c98429 
Signed-off-by: Prashant Gupta <[email protected]>
@prashantgupta24
🎨 add some longer chicken prompts
9b10ac2 
Signed-off-by: Prashant Gupta <[email protected]>
@prashantgupta24
♻️ use chicken prompts for deterministic output verification
f096f99 
Signed-off-by: Prashant Gupta <[email protected]>
@prashantgupta24
🎨 fmt
59ac876 
Signed-off-by: Prashant Gupta <[email protected]>
@prashantgupta24
🐛 whoops
d77b268 
Signed-off-by: Prashant Gupta <[email protected]>
prashantgupta24
prashantgupta24 approved these changes Jul 29, 2025
View reviewed changes
Copy link
Collaborator

@prashantgupta24 prashantgupta24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All tests passed on eager + spyre!

prashantgupta24
prashantgupta24 reviewed Jul 29, 2025
View reviewed changes
rafvasq
rafvasq approved these changes Jul 30, 2025
View reviewed changes
Copy link
Collaborator

@rafvasq rafvasq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @prashantgupta24!

joerunde
joerunde reviewed Jul 30, 2025
View reviewed changes
joerunde
joerunde reviewed Jul 30, 2025
View reviewed changes
joerunde
joerunde reviewed Jul 30, 2025
View reviewed changes
joerunde
joerunde reviewed Jul 30, 2025
View reviewed changes
joerunde
joerunde reviewed Jul 30, 2025
View reviewed changes
@prashantgupta24
🎨 some review comments
03f865f 
Signed-off-by: Prashant Gupta <[email protected]>
@prashantgupta24
🔥 whoops
54c9c3c 
Signed-off-by: Prashant Gupta <[email protected]>
@prashantgupta24
🎨 fmt
5b0953c 
Signed-off-by: Prashant Gupta <[email protected]>
@prashantgupta24 prashantgupta24 merged commit f0b3ead into main Jul 30, 2025
17 checks passed
@prashantgupta24 prashantgupta24 deleted the scheduling_tests_check_output branch July 30, 2025 17:34
@yannicks1 yannicks1 mentioned this pull request Aug 5, 2025
Merged
yannicks1 added a commit that referenced this pull request Aug 5, 2025
@yannicks1
[docs][CB] remove warning that no output correctness is asserted for …
8fab64f 
…scheduler step tests (#360)

### [docs][CB] remove warning that no output correctness is asserted for
scheduler step tests


we now do assert output correctness for the scheduling test since #337 ,
hence this warning should be removed.

Signed-off-by: Yannick Schnider <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joerunde joerunde joerunde left review comments

@prashantgupta24 prashantgupta24 prashantgupta24 approved these changes

@rafvasq rafvasq rafvasq approved these changes

@maxdebayser maxdebayser maxdebayser approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sducouedic @prashantgupta24 @maxdebayser @joerunde @rafvasq