-
Notifications
You must be signed in to change notification settings - Fork 67
Fix integration tests for streaming case #548
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
2793e48 to
6468b23
Compare
integration_tests/rest_api_utils.py
Outdated
| required_output_fields: Optional[List[str]], | ||
| response_text_regex: Optional[str], | ||
| ): | ||
| print("raw response", response) |
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.
remove this?
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.
we can; it's been helpful for debugging if the tests fail
| ) as response: | ||
| assert response.status == 200, (await response.read()).decode() | ||
| return await response.json() | ||
| return (await response.read()).decode() |
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.
hmm, how did this work before?
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'm pretty sure it didn't
integration_tests/rest_api_utils.py
Outdated
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.
should implement these?
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'm not really sure what this is supposed to be testing. the response for streaming is different from that for sync so the same checks don't really apply
44f30ca to
861fea3
Compare
Pull Request Summary
What is this PR changing? Why is this change being made? Any caveats you'd like to highlight? Link any relevant documents, links, or screenshots here if applicable.
vLLM integration tests for the stream case has a few bugs that this addresses
Test Plan and Usage Guide
How did you validate that your PR works correctly? How do you run or demo the code? Provide enough detail so a reviewer can reasonably reproduce the testing procedure. Paste example command line invocations if applicable.