[serve] Implement timeouts for streaming & enable all tests with experimental streaming flag turned on #36261

edoakes · 2023-06-09T15:45:10Z

Why are these changes needed?

This is the penultimate PR for turning RAY_SERVE_EXPERIMENTAL_STREAMING on by default.

In this change, we are now running all Serve tests under three conditions:

RAY_SERVE_EXPERIMENTAL_STREAMING=0 && RAY_SERVE_ENABLE_NEW_ROUTING=0
RAY_SERVE_EXPERIMENTAL_STREAMING=0 && RAY_SERVE_ENABLE_NEW_ROUTING=1
RAY_SERVE_EXPERIMENTAL_STREAMING=1 && RAY_SERVE_ENABLE_NEW_ROUTING=1

(Note that RAY_SERVE_EXPERIMENTAL_STREAMING=1 implies RAY_SERVE_ENABLE_NEW_ROUTING so the latter is not explicitly set).

This required making two functionality fixes:

Never using the streaming path for Java even if the flag is on.
Implementing timeouts in the streaming path.

I could split these into separate PRs, but they can't really be fully tested independently, so opting to combine them here.

Related issue number

Closes #35779

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
- I've added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

shrekris-anyscale

Nice, this change looks good to me overall!

shrekris-anyscale · 2023-06-29T16:34:54Z

python/ray/serve/_private/http_proxy.py

+ else:
+ curr_timeout_s = timeout_s - (time.time() - start)
+
+ obj_ref = await obj_ref_generator._next_async(timeout_s=curr_timeout_s)


What's obj_ref_generator's behavior if the timeout is negative? What if it's zero?

Good question, let me check and ensure we're being defensive here.

zero == timeout immediately, negative == indefinite. I made sure we only pass None or >=0

In this case, we don't need to have timeout_s < 0 checking?

I don't know what you mean. self.request_timeout_s will always be set to None if it's <0 in the constructor

Couldn't curr_timeout_s = timeout_s - (time.time() - start) make curr_timeout_s negative?

let me fix and add unit tests

good catch. I added this to utils.py::calculate_remaining_timeout and added a test case for it.

Nice, thanks!

GeneDer

Just a small comment on how we set timeout, but otherwise LGTM

python/ray/serve/_private/http_proxy.py

python/ray/serve/BUILD

python/ray/serve/_private/http_proxy.py

sihanwang41 · 2023-06-29T17:05:20Z

python/ray/serve/_private/http_proxy.py

+ else:
+ curr_timeout_s = timeout_s - (time.time() - start)
+
+ obj_ref = await obj_ref_generator._next_async(timeout_s=curr_timeout_s)


In this case, we don't need to have timeout_s < 0 checking?

sihanwang41 · 2023-06-29T17:08:03Z

python/ray/serve/_private/http_proxy.py

@@ -56,6 +58,7 @@
 "RAY_SERVE_HTTP_REQUEST_MAX_RETRIES cannot be negative."
 )

+TIMEOUT_ERROR_CODE = "timeout"


normally, the error code is a string of number? Is the timeout error code exposing to the users?

it's just for metrics. we already have one for disconnects (just below)

sihanwang41

thanks for addressing my comments!

GeneDer

edoakes · 2023-06-29T18:15:31Z

FYI to reviewers: the diff grew because I refactored the tests to pull the timeout tests into test_request_timeout and add a test for timing out while pending assignment.

Signed-off-by: Edward Oakes <[email protected]>

…aming-on-by-default

Signed-off-by: Edward Oakes <[email protected]>

…rimental streaming flag turned on (ray-project#36261) This is the penultimate PR for turning RAY_SERVE_EXPERIMENTAL_STREAMING on by default. In this change, we are now running all Serve tests under three conditions: - RAY_SERVE_EXPERIMENTAL_STREAMING=0 && RAY_SERVE_ENABLE_NEW_ROUTING=0 - RAY_SERVE_EXPERIMENTAL_STREAMING=0 && RAY_SERVE_ENABLE_NEW_ROUTING=1 - RAY_SERVE_EXPERIMENTAL_STREAMING=1 && RAY_SERVE_ENABLE_NEW_ROUTING=1 (Note that RAY_SERVE_EXPERIMENTAL_STREAMING=1 implies RAY_SERVE_ENABLE_NEW_ROUTING so the latter is not explicitly set). This required making two functionality fixes: - Never using the streaming path for Java even if the flag is on. - Implementing timeouts in the streaming path. I could split these into separate PRs, but they can't really be fully tested independently, so opting to combine them here. Signed-off-by: 久龙 <[email protected]>

…rimental streaming flag turned on (ray-project#36261) This is the penultimate PR for turning RAY_SERVE_EXPERIMENTAL_STREAMING on by default. In this change, we are now running all Serve tests under three conditions: - RAY_SERVE_EXPERIMENTAL_STREAMING=0 && RAY_SERVE_ENABLE_NEW_ROUTING=0 - RAY_SERVE_EXPERIMENTAL_STREAMING=0 && RAY_SERVE_ENABLE_NEW_ROUTING=1 - RAY_SERVE_EXPERIMENTAL_STREAMING=1 && RAY_SERVE_ENABLE_NEW_ROUTING=1 (Note that RAY_SERVE_EXPERIMENTAL_STREAMING=1 implies RAY_SERVE_ENABLE_NEW_ROUTING so the latter is not explicitly set). This required making two functionality fixes: - Never using the streaming path for Java even if the flag is on. - Implementing timeouts in the streaming path. I could split these into separate PRs, but they can't really be fully tested independently, so opting to combine them here. Signed-off-by: harborn <[email protected]>

…rimental streaming flag turned on (ray-project#36261) This is the penultimate PR for turning RAY_SERVE_EXPERIMENTAL_STREAMING on by default. In this change, we are now running all Serve tests under three conditions: - RAY_SERVE_EXPERIMENTAL_STREAMING=0 && RAY_SERVE_ENABLE_NEW_ROUTING=0 - RAY_SERVE_EXPERIMENTAL_STREAMING=0 && RAY_SERVE_ENABLE_NEW_ROUTING=1 - RAY_SERVE_EXPERIMENTAL_STREAMING=1 && RAY_SERVE_ENABLE_NEW_ROUTING=1 (Note that RAY_SERVE_EXPERIMENTAL_STREAMING=1 implies RAY_SERVE_ENABLE_NEW_ROUTING so the latter is not explicitly set). This required making two functionality fixes: - Never using the streaming path for Java even if the flag is on. - Implementing timeouts in the streaming path. I could split these into separate PRs, but they can't really be fully tested independently, so opting to combine them here.

…rimental streaming flag turned on (ray-project#36261) This is the penultimate PR for turning RAY_SERVE_EXPERIMENTAL_STREAMING on by default. In this change, we are now running all Serve tests under three conditions: - RAY_SERVE_EXPERIMENTAL_STREAMING=0 && RAY_SERVE_ENABLE_NEW_ROUTING=0 - RAY_SERVE_EXPERIMENTAL_STREAMING=0 && RAY_SERVE_ENABLE_NEW_ROUTING=1 - RAY_SERVE_EXPERIMENTAL_STREAMING=1 && RAY_SERVE_ENABLE_NEW_ROUTING=1 (Note that RAY_SERVE_EXPERIMENTAL_STREAMING=1 implies RAY_SERVE_ENABLE_NEW_ROUTING so the latter is not explicitly set). This required making two functionality fixes: - Never using the streaming path for Java even if the flag is on. - Implementing timeouts in the streaming path. I could split these into separate PRs, but they can't really be fully tested independently, so opting to combine them here. Signed-off-by: e428265 <[email protected]>

edoakes changed the title ~~[WIP] flip flag to see what fails~~ [serve] Enable all tests with experimental streaming flag turned on Jun 29, 2023

edoakes requested a review from a team June 29, 2023 15:35

shrekris-anyscale approved these changes Jun 29, 2023

View reviewed changes

GeneDer reviewed Jun 29, 2023

View reviewed changes

python/ray/serve/_private/http_proxy.py Outdated Show resolved Hide resolved

sihanwang41 reviewed Jun 29, 2023

View reviewed changes

sihanwang41 approved these changes Jun 29, 2023

View reviewed changes

GeneDer approved these changes Jun 29, 2023

View reviewed changes

edoakes changed the title ~~[serve] Enable all tests with experimental streaming flag turned on~~ [serve] Implement timeouts for streaming & enable all tests with experimental streaming flag turned on Jun 29, 2023

edoakes force-pushed the streaming-on-by-default branch 2 times, most recently from 5944681 to cd6f29a Compare June 30, 2023 16:46

edoakes requested review from zcin, architkulkarni and a team as code owners June 30, 2023 16:46

edoakes force-pushed the streaming-on-by-default branch from cd6f29a to e15bc5e Compare June 30, 2023 16:48

edoakes requested review from a team, amogkam, krfricke, richardliaw, matthewdeng, gjoliver, xwjiang2010, Yard1, maxpumperla, sven1977, avnishn, ArturNiederfahrenhorst, smorad, kouroshHakha and ericl as code owners June 30, 2023 16:48

edoakes removed request for wuisawesome, richardliaw, c21, scottjlee, architkulkarni, sven1977, gjoliver, amogkam, ArturNiederfahrenhorst, Yard1, krfricke, bveeramani, kouroshHakha, avnishn, DmitriGekhtman, xwjiang2010 and zcin June 30, 2023 16:50

edoakes force-pushed the streaming-on-by-default branch from 23d9d50 to 75f6e2f Compare June 30, 2023 16:51

edoakes added 6 commits June 30, 2023 11:52

squash everything

75f6e2f

Signed-off-by: Edward Oakes <[email protected]>

fix cyclic

732d31c

Signed-off-by: Edward Oakes <[email protected]>

skip

b38e378

Signed-off-by: Edward Oakes <[email protected]>

fix

1ec91d0

Signed-off-by: Edward Oakes <[email protected]>

Merge branch 'master' of https://github.com/ray-project/ray into stre…

0cbd876

…aming-on-by-default

fix

657e3eb

Signed-off-by: Edward Oakes <[email protected]>

edoakes merged commit f40d236 into ray-project:master Jun 30, 2023
30 of 35 checks passed

akshay-anyscale mentioned this pull request Jul 21, 2023

Add service deployment instructions to stable diffusion template #37645

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[serve] Implement timeouts for streaming & enable all tests with experimental streaming flag turned on #36261

[serve] Implement timeouts for streaming & enable all tests with experimental streaming flag turned on #36261

edoakes commented Jun 9, 2023 •

edited

Loading

shrekris-anyscale left a comment

shrekris-anyscale Jun 29, 2023

edoakes Jun 29, 2023

edoakes Jun 29, 2023

sihanwang41 Jun 29, 2023

edoakes Jun 29, 2023

shrekris-anyscale Jun 29, 2023

edoakes Jun 29, 2023

edoakes Jun 29, 2023

shrekris-anyscale Jun 29, 2023

GeneDer left a comment

sihanwang41 Jun 29, 2023

sihanwang41 Jun 29, 2023 •

edited

Loading

edoakes Jun 29, 2023

sihanwang41 left a comment

GeneDer left a comment

edoakes commented Jun 29, 2023

[serve] Implement timeouts for streaming & enable all tests with experimental streaming flag turned on #36261

[serve] Implement timeouts for streaming & enable all tests with experimental streaming flag turned on #36261

Conversation

edoakes commented Jun 9, 2023 • edited Loading

Why are these changes needed?

Related issue number

Checks

shrekris-anyscale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeneDer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sihanwang41 Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sihanwang41 left a comment

Choose a reason for hiding this comment

GeneDer left a comment

Choose a reason for hiding this comment

edoakes commented Jun 29, 2023

edoakes commented Jun 9, 2023 •

edited

Loading

sihanwang41 Jun 29, 2023 •

edited

Loading