[SkyServe] Rolling Update #2935

MaoZiming · 2024-01-03T01:12:40Z

Based on discussion with @cblmemo, rewrite sky serve update based on #2581

This PR introduces sky serve update CLI for sky serve.

Usage:

sky serve update <service-name> <new-task-yaml>

The autoscaler will perform a rolling update, gradually change your replicas to latest version.

Note to myself:

The current PR will launch a new replica even if the task yaml is the same. The reason is that the user workdir or the executable can change, and that might require re-launching the same program.

Tested (run the relevant ones):

Code formatting: bash format.sh
pytest tests/test_smoke.py -k 'test_skyserve_update'

cblmemo

Thanks!! This is amazing 🚀 Just done a pass and left some comments. One thing to notice is that on controller, we might want to store as less information in memory as possible and store in database instead.

cblmemo · 2024-01-04T08:25:05Z

sky/cli.py

+# which incorrectly recognizes the help string as a docstring.
+# pylint: disable=bad-docstring-quotes
+@click.option(
+    '--mixed-replica-version',


add a shortcut (say -m)?

Also, should we consider a more widely-adopted name? e.g. blue&green

added a shortcut. We should discuss the name to use, blue&green seems too specific?

Let's keep the current name for now and discuss more w/ other group members.

Based on offline discussion with @Michaelvll , let's not expose the flag to user, and set the default behavior to not mixing the replicas, and leave a Todo.

sky/cli.py

sky/serve/autoscalers.py

sky/serve/replica_managers.py

sky/serve/serve_utils.py

sky/serve/core.py

…o serve-update-new

sky/cli.py

cblmemo · 2024-01-12T16:32:14Z

sky/cli.py

+          'version become ready, direct traffic only to previous replicas '
+          'with same version and after that, direct traffic only to the '
+          'new replicas.'))
+def serve_update(


We should add resources override for this CLI too after #2979 is merged.

sky/cli.py

sky/serve/autoscalers.py

sky/serve/replica_managers.py

…o serve-update-new

cblmemo

Final comments; All of them are nits. It would be great if we could have some smoke tests and attach manual tests you've run to the PR description ;) Besides that it looks great on my side! 🙌🏻 @Michaelvll could you also take a look when you got time?

sky/cli.py

cblmemo · 2024-01-18T14:09:25Z

sky/serve/autoscalers.py

@@ -67,8 +67,19 @@ def __init__(self, spec: 'service_spec.SkyServiceSpec') -> None:
        """
        self.min_replicas: int = spec.min_replicas
        self.max_replicas: int = spec.max_replicas or spec.min_replicas
+        # Target number of replicas is initialized to min replicas.
+        # TODO(MaoZiming): add init replica numbers in SkyServe spec.


Feeling like updating init replica is an important feature; and the spot PR still takes some time to ship to master. cc @Michaelvll for a look

sky/serve/autoscalers.py

sky/serve/controller.py

sky/serve/serve_state.py

sky/serve/service.py

sky/serve/serve_utils.py

…o serve-update-new

Michaelvll

Thanks for adding this important feature @MaoZiming and thank you for the review @cblmemo! The code looks mostly good to me except some concurrency concerns and some questions. : )

sky/cli.py

Michaelvll · 2024-01-19T18:58:47Z

sky/serve/autoscalers.py

@@ -67,8 +67,19 @@ def __init__(self, spec: 'service_spec.SkyServiceSpec') -> None:
        """
        self.min_replicas: int = spec.min_replicas
        self.max_replicas: int = spec.max_replicas or spec.min_replicas
+        # Target number of replicas is initialized to min replicas.
+        # TODO(MaoZiming): add init replica numbers in SkyServe spec.


It is unclear to me what's difference between init_replicas vs min_replicas. I would prefer to reduce the specs in the arguments. Could we elaborate a bit for the TODO comment here for what the init_replicas is about and why we need it?

sky/serve/autoscalers.py

sky/serve/controller.py

Michaelvll · 2024-01-19T19:26:41Z

sky/serve/core.py

+        with ux_utils.print_exception_no_traceback():
+            raise RuntimeError(prompt)
+
+    version = service_record['version']


This can cause concurrency issue, e.g., if there are two updates called, they will have the same version number.

sky serve update

Before the first step updates the database on the controller, call sky serve update again.

We probably want to refer to the implementation of the job_id, where we first ask for the latest version+1 on the controller by using the atomic increment of the version in the database, so that multiple updates will have different version number and our update on the controller should only execute the update with the version number that is >= the version number in the database.

skypilot/sky/backends/cloud_vm_ray_backend.py

Lines 3332 to 3333 in c1f28bc

code = job_lib.JobLibCodeGen.add_job(job_name, username,

self.run_timestamp, resources_str)

skypilot/sky/skylet/job_lib.py

Lines 274 to 288 in c1f28bc

def add_job(job_name: str, username: str, run_timestamp: str,

resources_str: str) -> int:

"""Atomically reserve the next available job id for the user."""

job_submitted_at = time.time()

# job_id will autoincrement with the null value

_CURSOR.execute('INSERT INTO jobs VALUES (null, ?, ?, ?, ?, ?, ?, null, ?)',

(job_name, username, job_submitted_at, JobStatus.INIT.value,

run_timestamp, None, resources_str))

_CONN.commit()

rows = _CURSOR.execute('SELECT job_id FROM jobs WHERE run_timestamp=(?)',

(run_timestamp,))

for row in rows:

job_id = row[0]

assert job_id is not None

return job_id

skypilot/sky/skylet/job_lib.py

Line 60 in c1f28bc

job_id INTEGER PRIMARY KEY AUTOINCREMENT,

I see. Added

sky/serve/autoscalers.py

sky/serve/serve_state.py

…o serve-update-new

sky/serve/autoscalers.py

sky/serve/core.py

sky/serve/serve_utils.py

sky/serve/replica_managers.py

sky/serve/serve_state.py

Michaelvll · 2024-01-23T18:20:08Z

sky/serve/serve_state.py

+    # Check if the entry with the specified service_name and version exists
+    _DB.cursor.execute(
+        """SELECT * FROM versions WHERE service_name=? AND version=?""",
+        (service_name, version))
+    existing_entry = _DB.cursor.fetchone()
+
+    if existing_entry:
+        _DB.cursor.execute(
+            """\
+            UPDATE versions SET spec=?
+            WHERE service_name=? AND version=?""", (
+                pickle.dumps(spec),
+                service_name,
+                version,
+            ))
+    else:
+        _DB.cursor.execute(
+            """\
+            INSERT INTO versions
+            (service_name, version, spec)
+            VALUES (?, ?, ?)""", (service_name, version, pickle.dumps(spec)))
+    _DB.conn.commit()


Can we use INSERT or REPLACE?

skypilot/sky/global_user_state.py

Line 189 in f78a16e

'INSERT or REPLACE INTO clusters'

Actually, if we always call add_version before add the specs, why should we have the INSERT part?

Updated. The reason is that when service first initializes, we also need to add_version.

sky/serve/serve_state.py

Michaelvll

Thanks for the update @MaoZiming! It mostly looks good to me with some concerns with the concurrency. Please check the comments below

MaoZiming · 2024-01-23T19:55:07Z

@Michaelvll Thanks for the review! Addressed all the comments.

Michaelvll

Thanks @MaoZiming! This is awesome. It should be good to go once the tests passed. Also, can we add at least one smoke test for the functionality?

MaoZiming · 2024-01-23T22:16:14Z

@Michaelvll Thanks! added a new smoke test

MaoZiming added 2 commits January 2, 2024 20:11

rewrite skyserve update

b262a8f

remove sigint

282c942

MaoZiming requested a review from cblmemo January 3, 2024 15:49

delete storage buckets of older versions

cbae61d

MaoZiming requested a review from Michaelvll January 3, 2024 16:26

MaoZiming added 8 commits January 3, 2024 11:41

revert cleanup storage

0af3974

clean up storage from past versions

354351f

add a todo comment

6b32f01

add service field backward compatibility

b68314f

add service field backward compatibility

86da9f8

fix bug

dbb2576

remove stale comments

e874381

update comment

fda99bc

cblmemo mentioned this pull request Jan 9, 2024

[SkyServe] Optimize Rolling Update by not re-launching the resources #2956

Open

cblmemo reviewed Jan 9, 2024

View reviewed changes

cblmemo mentioned this pull request Jan 9, 2024

[SkyServe] Introducing sky serve update #2581

Closed

10 tasks

MaoZiming added 7 commits January 9, 2024 17:13

pr review

1cfe161

add versions to db

30ba404

refactor

35514f6

fix weird serve-update-new

e0d3739

del

9490b70

Merge branch 'master' of https://github.com/skypilot-org/skypilot int…

dd16801

…o serve-update-new

merge master

8460344

cblmemo reviewed Jan 12, 2024

View reviewed changes

sky/serve/replica_managers.py Show resolved Hide resolved

sky/serve/replica_managers.py Outdated Show resolved Hide resolved

MaoZiming added 2 commits January 12, 2024 11:03

pr review

8ec23c7

clip target_num_replicas

1e09372

Michaelvll force-pushed the master branch from 71213e5 to 9743aa0 Compare January 13, 2024 05:30

MaoZiming requested a review from cblmemo January 15, 2024 17:04

Merge branch 'master' of https://github.com/skypilot-org/skypilot int…

c58914d

…o serve-update-new

MaoZiming added 5 commits January 17, 2024 14:53

update comments

cbe087f

if statements

7b4ad84

move add_column_to_table place

48023b1

move graceful termination to _terminate_replica

77ee71c

move version checking code to _refresh_process_pool

d0edc69

cblmemo approved these changes Jan 18, 2024

View reviewed changes

MaoZiming added 7 commits January 18, 2024 10:08

Merge branch 'master' of https://github.com/skypilot-org/skypilot int…

a235cca

…o serve-update-new

code reviews

bd857a1

address code reviews

9b0c208

version updating outside the for loop

c5f7117

code review

a029cd2

clean up storage

52df1df

rename logger info

e6c2884

Michaelvll reviewed Jan 19, 2024

View reviewed changes

MaoZiming added 4 commits January 19, 2024 16:24

code change

01e0a57

update service atomically

919590b

fix bug

2c6babc

fix

876b35d

MaoZiming requested a review from Michaelvll January 22, 2024 20:59

MaoZiming added 2 commits January 23, 2024 09:30

Merge branch 'master' of https://github.com/skypilot-org/skypilot int…

e3b8770

…o serve-update-new

update variable names

6b5dd43

Michaelvll reviewed Jan 23, 2024

View reviewed changes

atomic update to increment version

83db967

Michaelvll approved these changes Jan 23, 2024

View reviewed changes

add skyserve update smoke test

47eff6a

MaoZiming merged commit 3765f03 into master Jan 24, 2024
19 checks passed

MaoZiming deleted the serve-update-new branch January 24, 2024 06:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SkyServe] Rolling Update #2935

[SkyServe] Rolling Update #2935

MaoZiming commented Jan 3, 2024 •

edited

cblmemo left a comment

cblmemo Jan 4, 2024

cblmemo Jan 9, 2024

MaoZiming Jan 9, 2024

cblmemo Jan 12, 2024

MaoZiming Jan 12, 2024

MaoZiming Jan 17, 2024

cblmemo Jan 12, 2024

cblmemo left a comment

cblmemo Jan 18, 2024

Michaelvll left a comment

Michaelvll Jan 19, 2024

Michaelvll Jan 19, 2024

MaoZiming Jan 20, 2024

Michaelvll Jan 23, 2024

Michaelvll Jan 23, 2024

MaoZiming Jan 23, 2024

Michaelvll left a comment

MaoZiming commented Jan 23, 2024

Michaelvll left a comment

MaoZiming commented Jan 23, 2024

	code = job_lib.JobLibCodeGen.add_job(job_name, username,
	self.run_timestamp, resources_str)

	def add_job(job_name: str, username: str, run_timestamp: str,
	resources_str: str) -> int:
	"""Atomically reserve the next available job id for the user."""
	job_submitted_at = time.time()
	# job_id will autoincrement with the null value
	_CURSOR.execute('INSERT INTO jobs VALUES (null, ?, ?, ?, ?, ?, ?, null, ?)',
	(job_name, username, job_submitted_at, JobStatus.INIT.value,
	run_timestamp, None, resources_str))
	_CONN.commit()
	rows = _CURSOR.execute('SELECT job_id FROM jobs WHERE run_timestamp=(?)',
	(run_timestamp,))
	for row in rows:
	job_id = row[0]
	assert job_id is not None
	return job_id

[SkyServe] Rolling Update #2935

[SkyServe] Rolling Update #2935

Conversation

MaoZiming commented Jan 3, 2024 • edited

cblmemo 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

cblmemo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michaelvll 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

Michaelvll left a comment

Choose a reason for hiding this comment

MaoZiming commented Jan 23, 2024

Michaelvll left a comment

Choose a reason for hiding this comment

MaoZiming commented Jan 23, 2024

MaoZiming commented Jan 3, 2024 •

edited