Skip to content
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.

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

[DO NOT MERGE] [API server] consistent module path of server and cli #4884

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aylei
Copy link
Collaborator

@aylei aylei commented Mar 5, 2025

close #4801

Tested manually, checked whether the module in current working dir is used:

# Note: avoid -e
$ pip install ".[all]"
$ sed -i "s/API_VERSION = '3'/API_VERSION = '9'/" sky/server/constants.py
$ sky api stop && sky api start

This branch, the installed module is used instead of current dir, which is expected:

$ sky api start
$ curl http://127.0.0.1:46580/api/health
{"status":"healthy","api_version":"3","commit":"296a22e868b9bdf1faccbe3effbfb858a5a05905-dirty","version":"1.0.0-dev0"}%

This branch, but launch with python -m sky.cli api start:

$ sky api start
# Both client and server are started using the same version from current dir, which is expected I think
$ curl http://127.0.0.1:46580/api/health
{"status":"healthy","api_version":"9","commit":"296a22e868b9bdf1faccbe3effbfb858a5a05905-dirty","version":"1.0.0-dev0"}%

Master branch, start raise an error, indicates the local dir is used:

sky.exceptions.ApiServerConnectionError: Could not connect to SkyPilot API server at http://127.0.0.1:46580. Please ensure that the server is running. Try: curl http://127.0.0.1:46580/api/health

During handling of the above exception, another exception occurred:

AssertionError: API server version mismatch when starting the server. Server version: 9 Client version: 3

Also tested the case that skyserver is not installed:

$ sky api start
Failed to connect to SkyPilot API server at http://127.0.0.1:46580. Starting a local server.
sky.exceptions.ApiServerConnectionError: Could not connect to SkyPilot API server at http://127.0.0.1:46580. Please ensure that the server is running. Try: curl http://127.0.0.1:46580/api/health

During handling of the above exception, another exception occurred:

RuntimeError: SkyPilot API server command skyserver is not available in PATH. Rerun `pip install -e .` to install the command.

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (see above)
  • 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

@aylei aylei marked this pull request as ready for review March 5, 2025 10:18
@aylei aylei requested review from cg505, Michaelvll and zpoint March 5, 2025 10:18
@aylei
Copy link
Collaborator Author

aylei commented Mar 5, 2025

/quicktest-core

1 similar comment
@aylei
Copy link
Collaborator Author

aylei commented Mar 5, 2025

/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.

I think this makes sense. @Michaelvll should also review.

@aylei aylei force-pushed the server-entrypoint branch from f4d47b2 to 5e6d00d Compare March 7, 2025 08:11
@aylei aylei requested review from Michaelvll and cg505 March 7, 2025 08:13
Signed-off-by: Aylei <[email protected]>
@aylei
Copy link
Collaborator Author

aylei commented Mar 7, 2025

Thanks for @Michaelvll 's idea, this PR is now greatly simplified and handles python -m sky.cli case correctly, ready for another look @cg505 @Michaelvll

Signed-off-by: Aylei <[email protected]>
@aylei aylei changed the title [API server] use console entrypoint to start server [API server] consistent module path of server and cli Mar 7, 2025
subprocess.Popen(cmd,
shell=True,
start_new_session=True,
cwd=working_dir)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh, this could be a bit dangerous, if a user specified a relative path for SKYPILOT_CONFIG=./config.yaml. Does this mean the config path will not be loaded locally?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could use python -P CLI option, but this is only available in 3.11 and higher.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great catch! Will investigate more

@aylei aylei changed the title [API server] consistent module path of server and cli [DO NOT MERGE] [API server] consistent module path of server and cli Mar 11, 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.

sky api start loads Python code from the current directory instead of the pip-installed directory
3 participants