-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[serve] Enable passing starlette requests w/ a warning #37046
Conversation
Signed-off-by: Edward Oakes <[email protected]>
Signed-off-by: Edward Oakes <[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, thanks Ed!
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 work!
@@ -45,6 +49,9 @@ | |||
|
|||
logger = logging.getLogger(SERVE_LOGGER_NAME) | |||
|
|||
# Used to only print a single warning when users pass starlette requests via handle. |
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.
# Used to only print a single warning when users pass starlette requests via handle. | |
# Used only to print a single warning when users pass starlette requests via handle. |
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.
The phrasing is as-is to specify that it's used "used to only print a single" versus printing multiple.
Not that this variable is "only used to print" a warning
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.
Oh got it, could we phrase it like this:
# Used to only print a single warning when users pass starlette requests via handle. | |
# Used to print only a single warning when users pass starlette requests via handle. |
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.
yeah not going to wait for another build for it though, if it fails i'll update it
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.
That sounds good.
Manually tested the microbenchmark locally, working |
…7046) In the new streaming codepath, the starlette.requests.Request object is no longer serializable so it cannot be passed directly via ServeHandle. This is an anti-pattern because the request may not be fully functioning (e.g., detecting disconnects won't work). However, some existing code may rely on it, so in this change we add support by buffering the requests but print a warning message to discourage usage. Signed-off-by: 久龙 <[email protected]>
…7046) In the new streaming codepath, the starlette.requests.Request object is no longer serializable so it cannot be passed directly via ServeHandle. This is an anti-pattern because the request may not be fully functioning (e.g., detecting disconnects won't work). However, some existing code may rely on it, so in this change we add support by buffering the requests but print a warning message to discourage usage. Signed-off-by: harborn <[email protected]>
…7046) In the new streaming codepath, the starlette.requests.Request object is no longer serializable so it cannot be passed directly via ServeHandle. This is an anti-pattern because the request may not be fully functioning (e.g., detecting disconnects won't work). However, some existing code may rely on it, so in this change we add support by buffering the requests but print a warning message to discourage usage.
…7046) In the new streaming codepath, the starlette.requests.Request object is no longer serializable so it cannot be passed directly via ServeHandle. This is an anti-pattern because the request may not be fully functioning (e.g., detecting disconnects won't work). However, some existing code may rely on it, so in this change we add support by buffering the requests but print a warning message to discourage usage. Signed-off-by: e428265 <[email protected]>
Why are these changes needed?
In the new streaming codepath, the
starlette.requests.Request
object is no longer serializable so it cannot be passed directly viaServeHandle
.This is an anti-pattern because the request may not be fully functioning (e.g., detecting disconnects won't work). However, some existing code may rely on it, so in this change we add support by buffering the requests but print a warning message to discourage usage.
Related issue number
#37027
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.