Skip to content
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

Downgrade sse-starlette version #478

Merged
merged 2 commits into from
Mar 28, 2024

Conversation

squeakymouse
Copy link
Contributor

@squeakymouse squeakymouse commented Mar 28, 2024

Pull Request Summary

The updated version has weird behavior with streaming in the http-forwarder (tokens getting streamed back were batched so time to first token was very long; we expect to get tokens back at a steady rate instead)

I tried sse-starlette version 1.8.2 to see if it was the major version bump (1.8.2 -> 2.0.0) that broke things, but 1.8.2 still had the weird behavior, so downgrading to the original version of 1.6.1 from before the security scan updates

Should still be fine with security scan

Test Plan and Usage Guide

Tested that using this image for the HTTP forwarder of a Llama 2 endpoint in the training cluster fixes the oncall issue of streaming time to first token being long (via curling localhost:5000 from the HTTP forwarder)

@squeakymouse squeakymouse requested a review from a team March 28, 2024 19:39
@ian-scale
Copy link
Collaborator

ian-scale commented Mar 28, 2024

Should still be fine with security scan

any way to confirm this for sure before merging?

@squeakymouse
Copy link
Contributor Author

sse-starlette doesn't show up on the original list of vulnerabilities (would link to it but public repo 😛 ); I updated it because starlette does, but I guess the old version of sse-starlette is still compatible with the updated starlette 🙂

@squeakymouse squeakymouse enabled auto-merge (squash) March 28, 2024 20:19
Copy link
Member

@seanshi-scale seanshi-scale left a comment

Choose a reason for hiding this comment

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

could you write how changes were tested?

Copy link
Collaborator

@edgan8 edgan8 left a comment

Choose a reason for hiding this comment

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

Talked with Katie, sse starlette was on an older version two weeks ago so this looks good. Could you add more details to the PR explaining the context for what broke and why we need this specific version?

@squeakymouse squeakymouse merged commit bdf4a25 into main Mar 28, 2024
5 checks passed
@squeakymouse squeakymouse deleted the katiewu/change-dependency-versions branch March 28, 2024 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants