-
Notifications
You must be signed in to change notification settings - Fork 26
[CB] Update CB docs + Refactoring scheduling step-by-step inference tests #323
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.
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
Conversation
Signed-off-by: Sophie du Couédic <[email protected]>
Signed-off-by: Sophie du Couédic <[email protected]>
Signed-off-by: Sophie du Couédic <[email protected]>
Signed-off-by: Sophie du Couédic <[email protected]>
|
👋 Hi! Thank you for contributing to vLLM support on Spyre. Or this can be done with Now you are good to go 🚀 |
Signed-off-by: Sophie du Couédic <[email protected]>
Signed-off-by: Sophie du Couédic <[email protected]>
Signed-off-by: Sophie du Couédic <[email protected]>
Signed-off-by: Sophie du Couédic <[email protected]>
Signed-off-by: Sophie du Couédic <[email protected]>
Signed-off-by: Sophie du Couédic <[email protected]>
|
I just noticed that now the cb scheduling tests are skipped, trying to figure out why is this 🤔 |
Signed-off-by: Sophie du Couédic <[email protected]>
Oh I found out, I didn't know unit tests had to be prefixed with |
|
thanks for being so reactive in your review for this big PR @prashantgupta24 , I'll finish addressing your comments tomorrow I see that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great restructuring Sophie! left a couple of minor comments.
| @@ -0,0 +1,10 @@ | |||
| # End output correctness tests | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we find a better title for this section? It would be nice to coincide with the chapter on the left of the page. Currently we have:
End-to-End -> title: End output correctness tests -> file name: e2e_testing.md
Step-by-Step -> title: Scheduler inference steps tests -> file name: scheduling_inference_steps.md
Other Tests -> title: Other tests -> file name: other_tests.md
what about:
Output Tests -> title: Output Tests -> file name: output_tests.md
Scheduler Tests -> title: Scheduler Tests -> file name: scheduler_tests.md
Other Tests -> title: Other tests -> file name: other_tests.md
e.g. making things more uniform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about scheduler_tests only because there is some other test test_cb_max_tokens in test_spyre_cb.py that is also testing the scheduler (but just the constraint), without really belonging to this test suite imo. I want to emphasis that this is somehow "step-by-step correct execution". And it won't be only about the scheduler correctness, on the compiler side it is also required that the compiled graph cab adapt to different scenarios of prompts_length/max_tokens/steps joining.
But I agree the current naming is a bit fuzzy, I changed it a lot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But output_test is great naming, I change that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the scheduler tests, I renamed everything to scheduler_steps tests 👍
8559b35 to
c7cc603
Compare
Signed-off-by: Sophie du Couédic <[email protected]>
Signed-off-by: Sophie du Couédic <[email protected]>
Signed-off-by: Sophie du Couédic <[email protected]>
Signed-off-by: Sophie du Couédic <[email protected]>
c7cc603 to
2b61721
Compare
Signed-off-by: Sophie du Couédic <[email protected]>
|
Last question of whether we want to integrate the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Two main changes in this PR:
test_scheduler_cb_steps_tkvto a helper function, and the previously parameters functions oftest_scheduler_cb_steps_tkvto unit testsCloses #318
Closes #319