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.

Sign up for GitHub

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

Jump to bottom

[serve] Set status message if deployment pending for too long #25861

Merged

edoakes merged 22 commits into ray-project:master from zcin:update_status_message

Jun 28, 2022

Contributor

zcin commented Jun 16, 2022 •

edited

Loading

Why are these changes needed?

If a ray cluster does not have enough resources for a serve deployment, the deployment will be stuck at updating status. This change will set the message field when allocations/initializations of actors have been pending for too long.

Related issue number

"Closes #25261"

Checks

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 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 :(

zcin added enhancement serve labels

zcin assigned shrekris-anyscale

zcin changed the title ~~Set status message if deployment pending for too long~~ [WIP][serve] Set status message if deployment pending for too long

zcin marked this pull request as ready for review

June 16, 2022 19:07

shrekris-anyscale reviewed

View reviewed changes

Contributor

shrekris-anyscale left a comment

Nice work so far! I added a comment about the comment.

python/ray/serve/deployment_state.py Outdated

    
                              if _SCALING_LOG_ENABLED:

                                  print_verbose_scaling_log()

                              # If status is UNHEALTHY, give it higher priority over the stuck

Contributor

shrekris-anyscale Jun 16, 2022

I think we can expand on this comment and its reasoning bit. Here's a suggestion:

Suggested change

      
                            # If status is UNHEALTHY, give it higher priority over the stuck
          
                            # If status is UNHEALTHY, leave the message as is. The issue that caused the deployment to be unhealthy should be prioritized over this resource availability issue.

This is a bit malformatted since it's one big line, so I recommend copying it into your local code and running the formatting script on it. Feel free to edit this comment further.

python/ray/serve/deployment_state.py Outdated

    
                                  f"Deployment '{self._name}' has "

                                  f"{len(pending_initialization)} replicas that have taken "

                                  f"more than {SLOW_STARTUP_WARNING_S}s to initialize. This "

                                  f"may be caused by a slow __init__ or reconfigure method."

                              )

                              logger.warning(message)

                              # If status is UNHEALTHY, give it higher priority over the stuck

Contributor

shrekris-anyscale Jun 16, 2022 •

edited

Loading

Same suggestion for this comment.

shrekris-anyscale requested changes

View reviewed changes

Contributor

shrekris-anyscale left a comment

Great work on adding the unit test! It looks good. I've added a few suggestions.

python/ray/serve/tests/test_standalone2.py Outdated

    
                      client._controller._get_slow_startup_warning_s.remote()

                  )

                  # Lower slow startup warning threshold to 1 second to reduce test duration

                  client._controller._set_slow_startup_warning_period_s.remote(1)

Contributor

shrekris-anyscale Jun 17, 2022

Suggested change

      
                client._controller._set_slow_startup_warning_period_s.remote(1)
          
                ray.get(client._controller._set_slow_startup_warning_period_s.remote(1))

remote() calls aren't necessarily executed immediately. They return a reference to the return value, so their actual execution can happen later. By calling ray.get() on this reference, we can be sure that after this line executes, the function has actually finished running.

python/ray/serve/tests/test_standalone2.py Outdated

    
                  )

                  # Lower slow startup warning threshold to 1 second to reduce test duration

                  client._controller._set_slow_startup_warning_period_s.remote(1)

                  client._controller._set_slow_startup_warning_s.remote(1)

Contributor

shrekris-anyscale Jun 17, 2022

Suggested change

      
                client._controller._set_slow_startup_warning_s.remote(1)
          
                ray.get(client._controller._set_slow_startup_warning_s.remote(1))

python/ray/serve/tests/test_standalone2.py Outdated

    
                  wait_for_condition(updating_message, timeout=2)

                  # Reset slow startup warning threshold in case bugs that cause different

                  # tests to share state occur

                  client._controller._set_slow_startup_warning_period_s.remote(

Contributor

shrekris-anyscale Jun 17, 2022

We should wrap this in ray.get().

python/ray/serve/tests/test_standalone2.py Outdated

    
                  client._controller._set_slow_startup_warning_period_s.remote(

                      original_slow_startup_warning_period_s

                  )

                  client._controller._set_slow_startup_warning_s.remote(

Contributor

shrekris-anyscale Jun 17, 2022

We should wrap this in ray.get().

python/ray/serve/tests/test_standalone2.py Outdated

    
                      )

                  wait_for_condition(updating_message, timeout=2)

                  # Reset slow startup warning threshold in case bugs that cause different

Contributor

shrekris-anyscale Jun 17, 2022

Nit– can we rephrase this comment to "Reset slow startup warning threshold to prevent state sharing across unit tests."

python/ray/serve/tests/test_standalone2.py Outdated Show resolved Hide resolved

zcin added 7 commits

June 17, 2022 15:57


          Set message field of status if allocations/initializations have been …

ff4e88f

…pending for too long


          format changes

9eb7c87


          Add unit test

8d43a1b


          change comments

b9bdb4c


          minor fixes

a5e5b88


          Add unit test for checking that an unhealthy status should not be ove…

885b722

…rrided with resource availability issue


          format changes

4e0d28b

shrekris-anyscale approved these changes

View reviewed changes

Contributor

shrekris-anyscale left a comment

Great work! This change is well-tested, and it will be very helpful when debugging resource shortages.

zcin changed the title ~~[WIP][serve] Set status message if deployment pending for too long~~ [serve] Set status message if deployment pending for too long

zcin added 7 commits

June 21, 2022 13:08


          move unit tests from test_standalone2 to test_standalone

44dff49


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

a8fccba

…te_status_message


          fix bug

d5d85ab


          increase timeout

b8fb9fe


          change timeout

077e42d


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

f9c7a26

…te_status_message


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

b4aa1ab

…te_status_message

simon-mo reviewed

View reviewed changes

python/ray/serve/controller.py Outdated

Comment on lines 168 to 178

+                  def _get_slow_startup_warning_period_s(self) -> float:
+                      return ray.serve.deployment_state.SLOW_STARTUP_WARNING_PERIOD_S
+                  def _get_slow_startup_warning_s(self) -> float:
+                      return ray.serve.deployment_state.SLOW_STARTUP_WARNING_S
+                  def _set_slow_startup_warning_period_s(self, period: float) -> None:
+                      ray.serve.deployment_state.SLOW_STARTUP_WARNING_PERIOD_S = period
+                  def _set_slow_startup_warning_s(self, time_limit: float) -> None:
+                      ray.serve.deployment_state.SLOW_STARTUP_WARNING_S = time_limit

Contributor

simon-mo Jun 23, 2022

looks like they are called in together. Can you group them into the one _get and one _set call?
do we need to configure them this way? How about support configure these via environment variable and and test with environment fixture (@shrekris-anyscale can help here)

Contributor

simon-mo Jun 23, 2022

additionally, for private apis, we want to extremely pedantic, in particular, adding a _for_testing suffix.

Contributor

edoakes Jun 23, 2022

modifying global variables is in general bad code smell/error prone. if we're going to do this, I'd prefer to make it a field of the controller or deployment state manager and pull the default from the constants. Also ok with Simon's suggestion to use env variables.

edoakes reviewed

View reviewed changes

Contributor

edoakes left a comment

Looks good aside from the getters/setters!

python/ray/serve/controller.py Outdated

Comment on lines 168 to 178

+                  def _get_slow_startup_warning_period_s(self) -> float:
+                      return ray.serve.deployment_state.SLOW_STARTUP_WARNING_PERIOD_S
+                  def _get_slow_startup_warning_s(self) -> float:
+                      return ray.serve.deployment_state.SLOW_STARTUP_WARNING_S
+                  def _set_slow_startup_warning_period_s(self, period: float) -> None:
+                      ray.serve.deployment_state.SLOW_STARTUP_WARNING_PERIOD_S = period
+                  def _set_slow_startup_warning_s(self, time_limit: float) -> None:
+                      ray.serve.deployment_state.SLOW_STARTUP_WARNING_S = time_limit

Contributor

edoakes Jun 23, 2022

modifying global variables is in general bad code smell/error prone. if we're going to do this, I'd prefer to make it a field of the controller or deployment state manager and pull the default from the constants. Also ok with Simon's suggestion to use env variables.

zcin added 3 commits

June 24, 2022 13:54


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

7096ce2

…te_status_message


          modify global variables through environment variables

de6cc2c


          format

6dc874c

shrekris-anyscale approved these changes

View reviewed changes

Contributor

shrekris-anyscale left a comment

Nice work! I added a couple suggestions to remove defaults from the tests. The fixture is getting the env vars and resetting them after the test. It doesn't need to introduce separate defaults.

python/ray/serve/tests/test_standalone.py Outdated Show resolved Hide resolved

python/ray/serve/tests/test_standalone.py Outdated Show resolved Hide resolved


          Update python/ray/serve/tests/test_standalone.py

e855501

Co-authored-by: shrekris-anyscale <[email protected]>


          Update python/ray/serve/tests/test_standalone.py

6e0e76d

Co-authored-by: shrekris-anyscale <[email protected]>

simon-mo reviewed

View reviewed changes

python/ray/serve/deployment_state.py Outdated

Comment on lines 74 to 77

+              SLOW_STARTUP_WARNING_S = int(os.getenv("SERVE_SLOW_STARTUP_WARNING_S", 30))
+              SLOW_STARTUP_WARNING_PERIOD_S = int(
+                  os.getenv("SERVE_SLOW_STARTUP_WARNING_PERIOD_S", 30)
+              )

Contributor

simon-mo Jun 27, 2022

Suggested change

      
            SLOW_STARTUP_WARNING_S = int(os.getenv("SERVE_SLOW_STARTUP_WARNING_S", 30))
          
            SLOW_STARTUP_WARNING_PERIOD_S = int(
          
                os.getenv("SERVE_SLOW_STARTUP_WARNING_PERIOD_S", 30)
          
            )
          
            SLOW_STARTUP_WARNING_S = int(os.environ.get("SERVE_SLOW_STARTUP_WARNING_S", 30))
          
            SLOW_STARTUP_WARNING_PERIOD_S = int(
          
                os.environ.get("SERVE_SLOW_STARTUP_WARNING_PERIOD_S", 30)
          
            )

In Serve codebase we uses os.environ, it would be great to keep them consistent so it makes our life easier in next few months during refactoring.

Contributor Author

zcin Jun 27, 2022 •

edited

Loading

Got it, thanks!

python/ray/serve/tests/test_standalone.py Outdated

Comment on lines 57 to 60

+                  original_slow_startup_warning_s = os.getenv("SERVE_SLOW_STARTUP_WARNING_S")
+                  original_slow_startup_warning_period_s = os.getenv(
+                      "SERVE_SLOW_STARTUP_WARNING_PERIOD_S"
+                  )

Contributor

simon-mo Jun 27, 2022

same comment for using os.environ

python/ray/serve/tests/test_standalone.py Outdated

Comment on lines 73 to 78

+                  # Reset slow startup warning threshold to prevent state sharing across unit
+                  # tests
+                  os.environ["SERVE_SLOW_STARTUP_WARNING_S"] = original_slow_startup_warning_s
+                  os.environ[
+                      "SERVE_SLOW_STARTUP_WARNING_PERIOD_S"
+                  ] = original_slow_startup_warning_period_s

Contributor

simon-mo Jun 27, 2022

please move these env meddling into a fixture https://docs.pytest.org/en/6.2.x/fixture.html#yield-fixtures-recommended

Contributor Author

zcin Jun 27, 2022

It's currently in a fixture, called lower_slow_startup_threshold_and_reset

zcin added 3 commits

June 27, 2022 10:23


          Merge branch 'update_status_message' of https://github.com/zcin/ray i…

f900d89

…nto update_status_message


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

d0bd51c

…te_status_message


          change method of getting environment variables

6bcb8c6

simon-mo approved these changes

View reviewed changes

edoakes merged commit da5366f into ray-project:master

shrekris-anyscale mentioned this pull request

[Serve] Set num_cpus to 0 in run_graph() task #26177

Merged

4 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement serve