Skip to content

Conversation

@cblmemo
Copy link
Collaborator

@cblmemo cblmemo commented Aug 7, 2025

Tested (run the relevant ones):

  • Code formatting: install pre-commit (auto-check on commit) or bash format.sh
  • Any manual or new tests for this PR (please specify below)
  • All smoke tests: /smoke-test (CI) or pytest tests/test_smoke.py (local)
  • Relevant individual tests: /smoke-test -k test_name (CI) or pytest tests/test_smoke.py::test_name (local)
  • Backward compatibility: /quicktest-core (CI) or pytest tests/smoke_tests/test_backward_compat.py (local)

@cblmemo cblmemo requested a review from cg505 August 7, 2025 23:52
@cblmemo
Copy link
Collaborator Author

cblmemo commented Aug 8, 2025

/smoke-test --serve
/quicktest-core

Copy link
Collaborator

@cg505 cg505 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any chance this runs at the same time as launch is still happening for the cluster? If yes we need to resolve that I think. If no, this should be good, left a minor comment.

replica_infos = serve_state.get_replica_infos(service_name)
info2proc: Dict[replica_managers.ReplicaInfo,
multiprocessing.Process] = dict()
# NOTE(dev): This relies on `sky/serve/serve_utils.py::
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a note in sky/serve/serve_utils pointing to this code as well?

@cblmemo
Copy link
Collaborator Author

cblmemo commented Aug 12, 2025

Is there any chance this runs at the same time as launch is still happening for the cluster? If yes we need to resolve that I think. If no, this should be good, left a minor comment.

No, as we will kill every children process before we calling this function:

subprocess_utils.kill_children_processes(
parent_pids=[process.pid for process in process_to_kill],
force=True)

@cblmemo cblmemo enabled auto-merge (squash) August 12, 2025 19:06
@cblmemo cblmemo merged commit ab2a877 into master Aug 12, 2025
15 checks passed
@cblmemo cblmemo deleted the serve-limit-down branch August 12, 2025 19:16
massaindustries pushed a commit to Seeweb/skypilot that referenced this pull request Aug 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants