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

[Serve] Return service name and endpoint from sky.serve.up #3546

Merged
merged 6 commits into from
May 16, 2024

Conversation

fozziethebeat
Copy link
Contributor

@fozziethebeat fozziethebeat commented May 14, 2024

Fixes #3545

This returns the generated (or specified) service name and the generated endpoint. This moves closer to supporting sky.serve.core.up as a library function.

Manually ran the following:

import sky
from sky.serve.service_spec import SkyServiceSpec 

run_str = """
docker run  \
--runtime nvidia \
--gpus all \
--shm-size 1g \
-v ~/.cache/huggingface:/root/.cache/huggingface \
--env "HUGGING_FACE_HUB_TOKEN=${HUGGINGFACE_TOKEN}" \
--ipc=host \
-p 8000:8000 \
vllm/vllm-openai:latest \
--trust-remote-code \                                                                            
--tensor-parallel-size $SKYPILOT_NUM_GPUS_PER_NODE \
--max-model-len 18000 \
--chat-template '<s>[INST] You are a fair judge assistant tasked with providing clear, objective feedback based on specific criteria, ensuring each assessment reflects the absolute standards set for performance. </s>{% for message in messages %}{% if message["role"] == "user" %}{{ "[INST] " + message["content"] + " [/INST]" }}{% elif message["role"] == "assistant" %}{{ message["content"] + "</s> " }}{% endif %}{% endfor %}' \
--model prometheus-eval/prometheus-7b-v2.0
"""
task = sky.Task(
    envs={
        "HUGGINGFACE_TOKEN": "<token>",
    },
    run=run_str,
)

task.set_service(SkyServiceSpec(
    "/v1/models",
    1200,
    1,
))
resources = sky.Resources(
    cloud=sky.clouds.AWS(),
    ports="8080",
    region="us-west-2",
    accelerators="A10G:1",
    instance_type="g5.8xlarge",
)
task.set_resources(resources)
service_name, endpoint = sky.serve.core.up(task)

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

Copy link
Collaborator

@cblmemo cblmemo left a comment

Choose a reason for hiding this comment

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

Thanks for adding this Feature! Left a comment about docstr 🫡

sky/serve/core.py Show resolved Hide resolved
Comment on lines 105 to 107

Returns:
A tuple with two values: the service name and the endpoint.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: is it possible to use a similar format with

skypilot/sky/execution.py

Lines 444 to 449 in 5a2f1b8

Returns:
job_id: Optional[int]; the job ID of the submitted job. None if the
backend is not CloudVmRayBackend, or no job is submitted to
the cluster.
handle: Optional[backends.ResourceHandle]; the handle to the cluster. None
if dryrun.

Also cc @Michaelvll for discussion: do you think it would be helpful to return the controller handle, like sky.launch?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It feels fine to me to only return the service name and the endpoint for now, as the controller handle can always be retrieved with sky.status.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

docstr change made~

Copy link
Collaborator

@Michaelvll Michaelvll left a comment

Choose a reason for hiding this comment

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

Thanks for adding the return value for up and sharing the use case @fozziethebeat! LGTM!

Just an FYI, we should be able to use sky.serve.up which is slightly simpler than sky.serve.core.up

@Michaelvll Michaelvll changed the title Return service name and endpoint from sky.serve.core.up [Serve] Return service name and endpoint from sky.serve.up May 16, 2024
@Michaelvll Michaelvll merged commit 1285e03 into skypilot-org:master May 16, 2024
20 checks passed
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.

Support sky.serve.core.up as a library function
3 participants