[Tests] Fix SkyServe Smoke Test #4566

cblmemo · 2025-01-15T18:53:35Z

This PR fixes several broken SkyServe smoke tests:

LLM tests failed cuz this breaking change in transformers: Breaking change in v4.48.0 and Python 3.9 huggingface/transformers#35639. Fix transformer version before this change.
test_skyserve_fast_update's port has been accidentally changed by [DigitalOcean] droplet integration #3832. Revert it back.
Replacing several workdir with wget to avoid storage creation.
Fixes [Tests][Serve] Smoke test on SkyServe has a flacky output check #4565 by waiting all provisioning resources has a vCPU=2 representation.
When the controller is put in INIT status by other services, the sky serve status --endpoint is likely to return a - due to the following check. This PR do retry on getting a - when fetching the endpoint.

Lines 793 to 798 in 0a810ee

    
           try: 
        
               endpoint = backend_utils.get_endpoints(handle.cluster_name, 
        
                                                      load_balancer_port).get( 
        
                                                          load_balancer_port, None) 
        
           except exceptions.ClusterNotUpError: 
        
               return '-'

An example:

+ export ORIGIN_SKYPILOT_DEBUG=$SKYPILOT_DEBUG; export SKYPILOT_DEBUG=0; endpoint=$(sky serve status --endpoint t-ss-new-autosca-6c-402b-64-rolling); until ! echo "$endpoint" | grep "Controller is initializing"; do echo "Waiting for serve endpoint to be ready..."; sleep 5; endpoint=$(sky serve status --endpoint t-ss-new-autosca-6c-402b-64-rolling); done; export SKYPILOT_DEBUG=$ORIGIN_SKYPILOT_DEBUG; echo "$endpoint"; s=$(curl http://$endpoint); echo "$s"; echo "$s" | grep "Hi, SkyPilot here"
-
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0curl: (6) Could not resolve host: -

Tested (run the relevant ones):

Code formatting: bash format.sh
Any manual or new tests for this PR (please specify below)
All smoke tests: pytest tests/test_smoke.py
Relevant individual smoke tests:
- pytest tests/test_smoke.py::test_skyserve_fast_update
- pytest tests/test_smoke.py::test_skyserve_base_ondemand_fallback
- pytest tests/test_smoke.py::test_skyserve_llm
- pytest tests/test_smoke.py::test_skyserve_new_autoscaler_update
Backward compatibility tests: conda deactivate; bash -i tests/backward_compatibility_tests.sh

Michaelvll · 2025-01-15T19:17:52Z

cc'ing @zpoint

Michaelvll · 2025-01-15T19:19:52Z

Note that there might be some fix attempts in #4548. Just want to make sure that we are not having conflicted fixes.

cblmemo · 2025-01-15T19:26:54Z

Note that there might be some fix attempts in #4548. Just want to make sure that we are not having conflicted fixes.

I took a quick look and it seems like this PR is just wrapping the tests with some wrappers? Should be good IIUC

cblmemo · 2025-01-15T19:40:57Z

/smoke-test serve

cblmemo · 2025-01-15T19:41:38Z

Local smoke test on relevant one passed. Running on buildkite now

cblmemo · 2025-01-16T03:26:04Z

@Michaelvll Seems like the smoke test is not passed on the buildkite due to this issue:

#3951 (comment)

I manually run the relevant smoke test and they passed. Could you help review this?

zpoint · 2025-01-16T03:58:10Z

Should be good to go, no conflicts right now.

Buildkite is experiencing some weird failures due to not separating different run environments and not using a clean environment to run jobs. I am working on fixing this.

Michaelvll

Thanks @cblmemo ! Left a comment below. mostly looks good to me

tests/smoke_tests/test_sky_serve.py

Michaelvll · 2025-01-16T03:58:04Z

tests/smoke_tests/test_sky_serve.py

+    '    sleep 5; '
+    '    s=$(sky serve status {name}); '
+    'done; '
+    # 2. Once controller is ready, check provisioning vs. vCPU=2


explain why we wait for the vCPU count int the comment? IIRC, if the replica is doing failover, the vCPU will show and disappear alternatively?

That is a good catch! The replica is unlikely to do failover for vCPU=2 resources. But i just realized that this only works for cpu replicas while the _SERVE_STATUS_WAIT is used for LLM replica as well (the llm test). Refactored to only include this in _check_replica_in_status. Re-running all smoke tests now.

zpoint · 2025-01-16T15:43:28Z

/smoke-test serve

cblmemo · 2025-01-17T00:57:31Z

Fixing another flaky bug:

When the controller is put in INIT status by other services, the sky serve status --endpoint is likely to return a - due to the following check. This PR do retry on getting a - when fetching the endpoint.

skypilot/sky/serve/serve_utils.py

Lines 793 to 798 in 0a810ee

    
           try: 
        
               endpoint = backend_utils.get_endpoints(handle.cluster_name, 
        
                                                      load_balancer_port).get( 
        
                                                          load_balancer_port, None) 
        
           except exceptions.ClusterNotUpError: 
        
               return '-'

An example:

+ export ORIGIN_SKYPILOT_DEBUG=$SKYPILOT_DEBUG; export SKYPILOT_DEBUG=0; endpoint=$(sky serve status --endpoint t-ss-new-autosca-6c-402b-64-rolling); until ! echo "$endpoint" | grep "Controller is initializing"; do echo "Waiting for serve endpoint to be ready..."; sleep 5; endpoint=$(sky serve status --endpoint t-ss-new-autosca-6c-402b-64-rolling); done; export SKYPILOT_DEBUG=$ORIGIN_SKYPILOT_DEBUG; echo "$endpoint"; s=$(curl http://$endpoint); echo "$s"; echo "$s" | grep "Hi, SkyPilot here"
-
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0curl: (6) Could not resolve host: -

zpoint · 2025-01-17T02:38:26Z

The buildkite is working now.

Failure on this build should reflect the real failure. Could u help confirm? @cblmemo

Feel free to comment /smoke-test serve trigger furthur build.

zpoint · 2025-01-17T04:34:56Z

/smoke-test serve

cblmemo · 2025-01-17T05:46:48Z

The buildkite is working now.

Failure on this build should reflect the real failure. Could u help confirm? @cblmemo

Feel free to comment /smoke-test serve trigger furthur build.

I found that a parameterized test function (e.g. test_skyserve_new_autoscaler_update) will run all of its tests at once, which will probably report partially correct information. Do you think we should separate them?

cblmemo · 2025-01-17T05:48:18Z

Also, seems like the buildkite is running on Azure which requires a little bit more initial delay. Just added.

zpoint · 2025-01-17T06:45:50Z

I will separate all parameter tests in #4548. That can also help separate test_skyserve_new_autoscaler_update to reduce flakiness.

However, currently, for test_skyserve_new_autoscaler_update to pass, both rolling and blue_green must pass. Separating them increases the chance of passing during a rerun, but it doesn’t help with the flakiness. It’s better if we find the root cause and fix it.😄

zpoint · 2025-01-17T08:34:37Z

/smoke-test serve

zpoint

Thanks @cblmemo This helps a lot in making buildkite 100% reliable !!!

tests/skyserve/update/new_autoscaler_before.yaml

cblmemo · 2025-01-17T18:29:30Z

/smoke-test serve

cblmemo · 2025-01-17T21:43:09Z

It seems like basically all test passed, except for:

test_skyserve_kubernetes_http on kubernetes, fixed by Fix type error -- Kubernetes sky down exception #4583;
test_skyserve_new_autoscaler_update, which composed of 2 parts and they have passed on their own, only not simultaneously in one run.

cc @Michaelvll for a look - i think it is ready to be merged ;)

Michaelvll

Thanks @cblmemo! LGTM.

cblmemo added 4 commits January 15, 2025 10:18

[Tests] Fix SkyServe Smoke Test

25233f2

fix llm

7b50ca9

comment

2fe25e3

add wait provisioning in _SERVE_STATUS_WAIT

b04b724

fix

d3a7caa

cblmemo marked this pull request as ready for review January 15, 2025 19:40

This was referenced Jan 15, 2025

[Serve] Refactor Autoscaler for better customizability. #4008

Merged

[Serve] HTTPS Support #3380

Merged

cblmemo requested review from zpoint and Michaelvll January 16, 2025 03:25

Michaelvll reviewed Jan 16, 2025

View reviewed changes

only apply waiting for _check_replica_in_status

69d3739

fix - for endpoint output

3a472fc

increase timeout

d462cad

zpoint mentioned this pull request Jan 17, 2025

more fix of smoke tests #4548

Merged

10 tasks

zpoint reviewed Jan 17, 2025

View reviewed changes

tests/skyserve/update/new_autoscaler_before.yaml Show resolved Hide resolved

increase initial delay

b0dd444

Michaelvll approved these changes Jan 17, 2025

View reviewed changes

cblmemo merged commit 6b23582 into master Jan 17, 2025
18 of 19 checks passed

cblmemo deleted the fix-serve-smoke branch January 17, 2025 23:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tests] Fix SkyServe Smoke Test #4566

[Tests] Fix SkyServe Smoke Test #4566

cblmemo commented Jan 15, 2025 •

edited

Loading

Michaelvll commented Jan 15, 2025

Michaelvll commented Jan 15, 2025 •

edited

Loading

cblmemo commented Jan 15, 2025

cblmemo commented Jan 15, 2025

cblmemo commented Jan 15, 2025

cblmemo commented Jan 16, 2025

zpoint commented Jan 16, 2025

Michaelvll left a comment

Michaelvll Jan 16, 2025

cblmemo Jan 17, 2025

zpoint commented Jan 16, 2025

cblmemo commented Jan 17, 2025

zpoint commented Jan 17, 2025

zpoint commented Jan 17, 2025

cblmemo commented Jan 17, 2025

cblmemo commented Jan 17, 2025

zpoint commented Jan 17, 2025 •

edited

Loading

zpoint commented Jan 17, 2025

zpoint left a comment •

edited

Loading

cblmemo commented Jan 17, 2025

cblmemo commented Jan 17, 2025

Michaelvll left a comment

	try:
	endpoint = backend_utils.get_endpoints(handle.cluster_name,
	load_balancer_port).get(
	load_balancer_port, None)
	except exceptions.ClusterNotUpError:
	return '-'

[Tests] Fix SkyServe Smoke Test #4566

[Tests] Fix SkyServe Smoke Test #4566

Conversation

cblmemo commented Jan 15, 2025 • edited Loading

Michaelvll commented Jan 15, 2025

Michaelvll commented Jan 15, 2025 • edited Loading

cblmemo commented Jan 15, 2025

cblmemo commented Jan 15, 2025

cblmemo commented Jan 15, 2025

cblmemo commented Jan 16, 2025

zpoint commented Jan 16, 2025

Michaelvll left a comment

Choose a reason for hiding this comment

Michaelvll Jan 16, 2025

Choose a reason for hiding this comment

cblmemo Jan 17, 2025

Choose a reason for hiding this comment

zpoint commented Jan 16, 2025

cblmemo commented Jan 17, 2025

zpoint commented Jan 17, 2025

zpoint commented Jan 17, 2025

cblmemo commented Jan 17, 2025

cblmemo commented Jan 17, 2025

zpoint commented Jan 17, 2025 • edited Loading

zpoint commented Jan 17, 2025

zpoint left a comment • edited Loading

Choose a reason for hiding this comment

cblmemo commented Jan 17, 2025

cblmemo commented Jan 17, 2025

Michaelvll left a comment

Choose a reason for hiding this comment

cblmemo commented Jan 15, 2025 •

edited

Loading

Michaelvll commented Jan 15, 2025 •

edited

Loading

zpoint commented Jan 17, 2025 •

edited

Loading

zpoint left a comment •

edited

Loading