-
Notifications
You must be signed in to change notification settings - Fork 26
🐛 fix a bug in tests, add DISABLE_ASSERTS #375
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
|
👋 Hi! Thank you for contributing to vLLM support on Spyre. Or this can be done with Now you are good to go 🚀 |
| from spyre_util import get_spyre_backend_list, get_spyre_model_list | ||
|
|
||
|
|
||
| def _check_result(client, model, max_tokens=8, temperature=0.0, n=1) -> None: |
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.
🌶️ !
|
bot:test |
Signed-off-by: Prashant Gupta <[email protected]>
7406f15 to
eeb87f0
Compare
|
bot:test |
Signed-off-by: Prashant Gupta <[email protected]>
| "VLLM_SPYRE_DYNAMO_BACKEND": backend | ||
| } | ||
| server_args = [ | ||
| server_args.extend([ |
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.
found a 🐛 !
Signed-off-by: Prashant Gupta <[email protected]>
Signed-off-by: Prashant Gupta <[email protected]>
Signed-off-by: Prashant Gupta <[email protected]>
|
just seen this... the modification to the model runner can be removed (see #384). The two internal issues on the tracker were coupled I guess. does this solve the remaining part of (internal) issue 977? |
Signed-off-by: Prashant Gupta <[email protected]>
Signed-off-by: Prashant Gupta <[email protected]>
Signed-off-by: Prashant Gupta <[email protected]>
Signed-off-by: Prashant Gupta <[email protected]>
with vllm_spyre:0.7.0
```
tests/e2e/test_spyre_basic.py::test_output[max_num_seqs(4)-sendnn-TP(4)-ibm-granite/granite-3.3-8b-instruct-cb
```
fails with
```
(VllmWorker rank=1 pid=62989) ERROR 08-15 17:09:36 [multiproc_executor.py:522] File "/usr/local/lib/python3.12/site-packages/torch_sendnn/backends.py", line 2440, in update_graph
(VllmWorker rank=1 pid=62989) ERROR 08-15 17:09:36 [multiproc_executor.py:522] self.meta["gl_op"] = GraphLoaderOp(self.meta["g2"], self)
(VllmWorker rank=1 pid=62989) ERROR 08-15 17:09:36 [multiproc_executor.py:522] ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(VllmWorker rank=1 pid=62989) ERROR 08-15 17:09:36 [multiproc_executor.py:522] File "/usr/local/lib/python3.12/site-packages/torch_sendnn/backends.py", line 1819, in __init__
(VllmWorker rank=1 pid=62989) ERROR 08-15 17:09:36 [multiproc_executor.py:522] raise RuntimeError(f"GraphLoader CompileGraph failed: {s}")
(VllmWorker rank=1 pid=62989) ERROR 08-15 17:09:36 [multiproc_executor.py:522] RuntimeError: GraphLoader CompileGraph failed: compile_graph failed
```
so we don't need specific tests!
Signed-off-by: Prashant Gupta <[email protected]>
Signed-off-by: Prashant Gupta <[email protected]>
Signed-off-by: Prashant Gupta <[email protected]>
|
Thanks for rename the title, it is much better! 😄 |
|
Can you add more information in the PR description as to why the DISABLE_ASSERT is needed? |
We already have it in |
Signed-off-by: Prashant Gupta <[email protected]>
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.
LGTM
Description
DISABLE_ASSERTSto the scheduling tests - It's very useful if you want to debug without actually asserting the valuesserver_args.extendshould have be called in tests