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

[SkyServe] Add service schema and change to new service YAML #2267

Closed
wants to merge 8 commits into from

Conversation

cblmemo
Copy link
Collaborator

@cblmemo cblmemo commented Jul 18, 2023

This PR adds a schema checking for our newer version service YAML, and change to new YAML config pattern.

An example:

service:
  port: 8082
  readiness_probe:
    path: /health
    timeout: 1200
  replica_policy:
    min_replica: 1
    max_replica: 5             # optional
    qpm_upper_threshold: 10    # optional, if qpm is higher, scale up
    qpm_lower_threshold: 1     # optional, if qpm is lower, scale down

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: bash tests/backward_comaptibility_tests.sh

@cblmemo cblmemo changed the base branch from master to endpoint July 18, 2023 20:59
@cblmemo cblmemo changed the title [SkyServe] Add service schema [SkyServe] Add service schema and change to new service YAML Jul 18, 2023
@cblmemo cblmemo requested a review from infwinston July 19, 2023 15:44
@cblmemo cblmemo mentioned this pull request Jul 19, 2023
5 tasks
Copy link
Member

@infwinston infwinston left a comment

Choose a reason for hiding this comment

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

Thanks @cblmemo ! The code looks great and I left a few comments.

sky/serve/autoscalers.py Show resolved Hide resolved
sky/serve/autoscalers.py Outdated Show resolved Hide resolved
sky/serve/autoscalers.py Outdated Show resolved Hide resolved
@@ -137,8 +140,9 @@ def evaluate_scaling(self):
self.scale_up(1)
self.last_scale_operation = current_time
elif requests_per_node > self.upper_threshold:
Copy link
Member

Choose a reason for hiding this comment

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

what if self.upper_threshold is None, then we shouldn't run the below code right?
we probably should refactor the code a bit to incorporate fix-node policy and autoscaling policy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure.. I'll just check if it is None for now, then if user only specify upper_threshold, that means user only want to autoscale up and #replica will only increase until reach max_replica; vise versa for only specify lower_threshold case. Only if user specified both upper and lower threshold will enable auto sale up/down. We might need a discussion on here.

sky/serve/common.py Outdated Show resolved Hide resolved
sky/serve/examples/http_server/task.yaml Outdated Show resolved Hide resolved
sky/serve/load_balancers.py Outdated Show resolved Hide resolved
sky/utils/schemas.py Outdated Show resolved Hide resolved
@cblmemo
Copy link
Collaborator Author

cblmemo commented Jul 20, 2023

Close and move to #2276.

@cblmemo cblmemo closed this Jul 20, 2023
@cblmemo cblmemo deleted the add-service-schema branch July 27, 2023 23:20
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.

2 participants