[Core] Remove backward compatibility code for 0.6.0 & 0.7.0 #4175

cblmemo · 2024-10-25T00:42:12Z

Try to remve backward compatibility code for 0.6.0 & 0.7.0

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_fill_in_the_name
Backward compatibility tests: conda deactivate; bash -i tests/backward_compatibility_tests.sh

cg505 · 2024-10-25T03:49:11Z

I pushed a commit that should cover the 0.7.0 removals.

romilbhardwaj

Thanks @cblmemo and @cg505! @cblmemo, could you help test this PR with relevant smoke tests + backward compatibility tests?

sky/serve/serve_state.py

romilbhardwaj · 2024-10-25T16:02:01Z

sky/serve/serve_state.py

@@ -34,7 +34,7 @@ def _get_db_path() -> str:
 def create_table(cursor: 'sqlite3.Cursor', conn: 'sqlite3.Connection') -> None:
    """Creates the service and replica tables if they do not exist."""

-    # auto_restart column is deprecated.
+    # auto_restart adn requested_resources column is deprecated.


Should we also remove the auto_restart and requested_resources columns when creating the table?

That would break the logic here:

skypilot/sky/serve/serve_state.py

Lines 324 to 348 in ee708e7

def _get_service_from_row(row) -> Dict[str, Any]:

(current_version, name, controller_job_id, controller_port,

load_balancer_port, status, uptime, policy, _, requested_resources,

requested_resources_str, _, active_versions) = row[:13]

return {

'name': name,

'controller_job_id': controller_job_id,

'controller_port': controller_port,

'load_balancer_port': load_balancer_port,

'status': ServiceStatus[status],

'uptime': uptime,

'policy': policy,

# The version of the autoscaler/replica manager are on. It can be larger

# than the active versions as the load balancer may not consider the

# latest version to be active for serving traffic.

'version': current_version,

# The versions that is active for the load balancer. This is a list of

# integers in json format. This is mainly for display purpose.

'active_versions': json.loads(active_versions),

# TODO(tian): Backward compatibility.

# Remove after 2 minor release, 0.6.0.

'requested_resources': pickle.loads(requested_resources)

if requested_resources is not None else None,

'requested_resources_str': requested_resources_str,

}

I think managed spot also keeps those columns for similar reasons:

skypilot/sky/jobs/state.py

Lines 41 to 69 in ee708e7

# === Database schema ===

# `spot` table contains all the finest-grained tasks, including all the

# tasks of a managed job (called spot for legacy reason, as it is generalized

# from the previous managed spot jobs). All tasks of the same job will have the

# same `spot_job_id`.

# The `job_name` column is now deprecated. It now holds the task's name, i.e.,

# the same content as the `task_name` column.

# The `job_id` is now not really a job id, but a only a unique

# identifier/primary key for all the tasks. We will use `spot_job_id`

# to identify the spot job.

# TODO(zhwu): schema migration may be needed.

_CURSOR.execute("""\

CREATE TABLE IF NOT EXISTS spot (

job_id INTEGER PRIMARY KEY AUTOINCREMENT,

job_name TEXT,

resources TEXT,

submitted_at FLOAT,

status TEXT,

run_timestamp TEXT CANDIDATE KEY,

start_at FLOAT DEFAULT NULL,

end_at FLOAT DEFAULT NULL,

last_recovered_at FLOAT DEFAULT -1,

recovery_count INTEGER DEFAULT 0,

job_duration FLOAT DEFAULT 0,

failure_reason TEXT,

spot_job_id INTEGER,

task_id INTEGER DEFAULT 0,

task_name TEXT)""")

_CONN.commit()

Co-authored-by: Romil Bhardwaj <romil.bhardwaj@berkeley.edu>

cblmemo · 2024-10-25T19:39:02Z

Also removed several other backward compatibility items. PTAL. Running tests now

cg505 · 2024-10-25T19:45:06Z

@cblmemo I did not remove the ones that said "after 0.7.0" since I assume that meant they should be kept in for 0.7.0 and removed for the next release. @Michaelvll @romilbhardwaj what did you intend here?

romilbhardwaj · 2024-10-25T19:57:24Z

Thanks @cblmemo and @cg505. I think it's fine to remove those which said "after 0.7.0" - the intent was "in 0.7.0". We should be more specific in our comments in the future.

One quick note @cblmemo - for using facing APIs that we are removing (auto_restart, qps_upper_threshold, qps_lower_threshold and instance_tags in config.yaml), can we add a quick error message that these fields have been removed (and suggest alternatives, e.g., using labels instead of instance_tags)?

cblmemo · 2024-10-25T20:02:17Z

Thanks @cblmemo and @cg505. I think it's fine to remove those which said "after 0.7.0" - the intent was "in 0.7.0". We should be more specific in our comments in the future.

One quick note @cblmemo - for using facing APIs that we are removing (auto_restart, qps_upper_threshold, qps_lower_threshold and instance_tags in config.yaml), can we add a quick error message that these fields have been removed (and suggest alternatives, e.g., using labels instead of instance_tags)?

SG! QQ: Does those error messages also need to be removed after 2 minor release, eg. 0.9.0?

romilbhardwaj · 2024-10-25T20:06:54Z

Probably not after 2 minor releases - removal messages should stay for some time. If we can have them tracked in a dictionary (e.g., something like removed_fields) along with a comment which version they were removed in (e.g., v0.7.0), then we can manually decide when to remove them.

cblmemo · 2024-10-25T20:12:15Z

Thanks @cblmemo and @cg505. I think it's fine to remove those which said "after 0.7.0" - the intent was "in 0.7.0". We should be more specific in our comments in the future.

One quick note @cblmemo - for using facing APIs that we are removing (auto_restart, qps_upper_threshold, qps_lower_threshold and instance_tags in config.yaml), can we add a quick error message that these fields have been removed (and suggest alternatives, e.g., using labels instead of instance_tags)?

Oh actually, those serve related interface have been disabled for 2 minor versions (and suggested to change to other API), and the part of code I removed is exactly those "quick error message", so I think it is the time to remove those quick error msg now? Not sure about the instance tags though.

cblmemo · 2024-10-25T20:12:49Z

We could discuss on whether to remove those API or not in this release :)

cblmemo · 2024-10-25T20:52:41Z

I tried backward compatibility tests and found an error on latest master - issued in #4179. ~~@romilbhardwaj did you encountered similar behaviour before?~~ Edited: Fixed in #4178. Will try running those tests again after #4178 is merged.

cblmemo · 2024-10-25T23:32:15Z

TODO: revert instance_tag and move to 0.8.0

This reverts commit 34c28e9.

…patibility

romilbhardwaj

Thanks @cblmemo and @cg505! lgtm. @cblmemo, please merge after tests pass.

cblmemo · 2024-10-28T19:51:46Z

Thanks @cblmemo and @cg505! lgtm. @cblmemo, please merge after tests pass.

Thanks @romilbhardwaj ! The backward compatibility test just passed. Do you think we need smoke test as well?

cblmemo · 2024-10-28T20:06:50Z

After an offline discussion we determined to merging now 🫡

[Core] Remove backward compatibility code for 0.6.0

d7a23ac

cblmemo modified the milestones: 7.0, v0.7 Oct 25, 2024

cblmemo requested a review from Michaelvll October 25, 2024 00:42

remove backwards compatibility for 0.7.0 release

3603241

romilbhardwaj reviewed Oct 25, 2024

View reviewed changes

cblmemo and others added 2 commits October 25, 2024 12:30

Update sky/serve/serve_state.py

c9bcf25

Co-authored-by: Romil Bhardwaj <romil.bhardwaj@berkeley.edu>

remove more

34c28e9

cblmemo changed the title ~~[Core] Remove backward compatibility code for 0.6.0~~ [Core] Remove backward compatibility code for 0.6.0 & 0.7.0 Oct 25, 2024

cblmemo added 3 commits October 25, 2024 16:37

Revert "remove more"

bba201d

This reverts commit 34c28e9.

remove more but not instance tags

b2a73f6

Merge remote-tracking branch 'origin/master' into delete-backward-com…

0134999

…patibility

romilbhardwaj approved these changes Oct 28, 2024

View reviewed changes

cblmemo added this pull request to the merge queue Oct 28, 2024

Merged via the queue into master with commit 546cb17 Oct 28, 2024
20 checks passed

cblmemo deleted the delete-backward-compatibility branch October 28, 2024 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Remove backward compatibility code for 0.6.0 & 0.7.0 #4175

[Core] Remove backward compatibility code for 0.6.0 & 0.7.0 #4175

cblmemo commented Oct 25, 2024 •

edited

Loading

cg505 commented Oct 25, 2024

romilbhardwaj left a comment

romilbhardwaj Oct 25, 2024

cblmemo Oct 25, 2024

cblmemo commented Oct 25, 2024

cg505 commented Oct 25, 2024

romilbhardwaj commented Oct 25, 2024

cblmemo commented Oct 25, 2024 •

edited

Loading

romilbhardwaj commented Oct 25, 2024

cblmemo commented Oct 25, 2024

cblmemo commented Oct 25, 2024

cblmemo commented Oct 25, 2024 •

edited

Loading

cblmemo commented Oct 25, 2024 •

edited

Loading

romilbhardwaj left a comment

cblmemo commented Oct 28, 2024

cblmemo commented Oct 28, 2024

	def _get_service_from_row(row) -> Dict[str, Any]:
	(current_version, name, controller_job_id, controller_port,
	load_balancer_port, status, uptime, policy, _, requested_resources,
	requested_resources_str, _, active_versions) = row[:13]
	return {
	'name': name,
	'controller_job_id': controller_job_id,
	'controller_port': controller_port,
	'load_balancer_port': load_balancer_port,
	'status': ServiceStatus[status],
	'uptime': uptime,
	'policy': policy,
	# The version of the autoscaler/replica manager are on. It can be larger
	# than the active versions as the load balancer may not consider the
	# latest version to be active for serving traffic.
	'version': current_version,
	# The versions that is active for the load balancer. This is a list of
	# integers in json format. This is mainly for display purpose.
	'active_versions': json.loads(active_versions),
	# TODO(tian): Backward compatibility.
	# Remove after 2 minor release, 0.6.0.
	'requested_resources': pickle.loads(requested_resources)
	if requested_resources is not None else None,
	'requested_resources_str': requested_resources_str,
	}

	# === Database schema ===
	# `spot` table contains all the finest-grained tasks, including all the
	# tasks of a managed job (called spot for legacy reason, as it is generalized
	# from the previous managed spot jobs). All tasks of the same job will have the
	# same `spot_job_id`.
	# The `job_name` column is now deprecated. It now holds the task's name, i.e.,
	# the same content as the `task_name` column.
	# The `job_id` is now not really a job id, but a only a unique
	# identifier/primary key for all the tasks. We will use `spot_job_id`
	# to identify the spot job.
	# TODO(zhwu): schema migration may be needed.
	_CURSOR.execute("""\
	CREATE TABLE IF NOT EXISTS spot (
	job_id INTEGER PRIMARY KEY AUTOINCREMENT,
	job_name TEXT,
	resources TEXT,
	submitted_at FLOAT,
	status TEXT,
	run_timestamp TEXT CANDIDATE KEY,
	start_at FLOAT DEFAULT NULL,
	end_at FLOAT DEFAULT NULL,
	last_recovered_at FLOAT DEFAULT -1,
	recovery_count INTEGER DEFAULT 0,
	job_duration FLOAT DEFAULT 0,
	failure_reason TEXT,
	spot_job_id INTEGER,
	task_id INTEGER DEFAULT 0,
	task_name TEXT)""")
	_CONN.commit()

[Core] Remove backward compatibility code for 0.6.0 & 0.7.0 #4175

[Core] Remove backward compatibility code for 0.6.0 & 0.7.0 #4175

Conversation

cblmemo commented Oct 25, 2024 • edited Loading

cg505 commented Oct 25, 2024

romilbhardwaj left a comment

Choose a reason for hiding this comment

romilbhardwaj Oct 25, 2024

Choose a reason for hiding this comment

cblmemo Oct 25, 2024

Choose a reason for hiding this comment

cblmemo commented Oct 25, 2024

cg505 commented Oct 25, 2024

romilbhardwaj commented Oct 25, 2024

cblmemo commented Oct 25, 2024 • edited Loading

romilbhardwaj commented Oct 25, 2024

cblmemo commented Oct 25, 2024

cblmemo commented Oct 25, 2024

cblmemo commented Oct 25, 2024 • edited Loading

cblmemo commented Oct 25, 2024 • edited Loading

romilbhardwaj left a comment

Choose a reason for hiding this comment

cblmemo commented Oct 28, 2024

cblmemo commented Oct 28, 2024

cblmemo commented Oct 25, 2024 •

edited

Loading

cblmemo commented Oct 25, 2024 •

edited

Loading

cblmemo commented Oct 25, 2024 •

edited

Loading

cblmemo commented Oct 25, 2024 •

edited

Loading