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] Add policy config to AutoscalingConfig #42072

Merged
merged 13 commits into from
Jan 8, 2024

Conversation

GeneDer
Copy link
Contributor

@GeneDer GeneDer commented Dec 21, 2023

Why are these changes needed?

Move BasicAutoscalingPolicy to a public file and add the new policy config to AutoscalingConfig.

Related issue number

First PR for #41135

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@GeneDer GeneDer marked this pull request as ready for review December 21, 2023 20:56
@GeneDer GeneDer requested a review from a team December 21, 2023 20:57
@GeneDer GeneDer self-assigned this Dec 21, 2023
@GeneDer GeneDer requested a review from edoakes January 2, 2024 17:54
@GeneDer
Copy link
Contributor Author

GeneDer commented Jan 2, 2024

@edoakes would be nice if you can give this a look as well

Copy link
Contributor

@shrekris-anyscale shrekris-anyscale left a comment

Choose a reason for hiding this comment

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

Nice work! I left some comments.

@edoakes
Copy link
Contributor

edoakes commented Jan 2, 2024

Mostly concerned about this base64 encoding and dynamic behavior of the policy field.

If something nonstandard like this is required in a PR I expect it to be called out in the description to let reviewers know why it's necessary and what alternatives there are, if any.

@GeneDer
Copy link
Contributor Author

GeneDer commented Jan 3, 2024

@edoakes @shrekris-anyscale I have addressed all the comments and removed the need to serialize policy with base64 (thanks to the idea on separating them into 2 fields). PTAL again when you have a sec!

@edoakes edoakes merged commit 615b0ab into ray-project:master Jan 8, 2024
2 checks passed
@GeneDer GeneDer deleted the add-autoscaling-policy-config branch January 8, 2024 17:03
vickytsang pushed a commit to ROCm/ray that referenced this pull request Jan 12, 2024
Move BasicAutoscalingPolicy to a public file and add the new policy config to AutoscalingConfig.

---------

Signed-off-by: Gene Su <[email protected]>
zcin pushed a commit to zcin/ray that referenced this pull request Feb 6, 2024
Move BasicAutoscalingPolicy to a public file and add the new policy config to AutoscalingConfig.

---------

Signed-off-by: Gene Su <[email protected]>
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