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] [KubeRay] Add flag that allows serve build to print Kubernetes-formatted output #28918

Merged
merged 15 commits into from
Oct 6, 2022

Conversation

shrekris-anyscale
Copy link
Contributor

@shrekris-anyscale shrekris-anyscale commented Sep 30, 2022

Why are these changes needed?

serve build lets users autogenerate a config file for their Serve deployment graph. Serve natively requires the config file to use camel casing for its keys. However, the KubeRay config requires camel-cased keys. This makes it difficult for users to autogenerate a config for Kubernetes.

This change adds the --kuberenets-format/-k option, which generates a Serve config with camel-cased keys, so it can be more easily copy-pasted into the Kubernetes config.

Related issue number

Closes #28831.

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 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
      • This change adds unit tests for helper functions and serve build.

Signed-off-by: Shreyas Krishnaswamy <[email protected]>
Signed-off-by: Shreyas Krishnaswamy <[email protected]>
Signed-off-by: Shreyas Krishnaswamy <[email protected]>
Signed-off-by: Shreyas Krishnaswamy <[email protected]>
Signed-off-by: Shreyas Krishnaswamy <[email protected]>
Signed-off-by: Shreyas Krishnaswamy <[email protected]>
@@ -478,6 +487,24 @@ def build(import_path: str, app_dir: str, output_path: Optional[str]):
).dict(exclude_defaults=True)
)

if kubernetes_format:
print(
"NOTE: Kubernetes requires the following fields to be "
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this warning can be worded more concisely and clearly. Does anyone have suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just json serialize it for the user instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, it would be great to remove this manual action from the customer side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could json serialize it. The output would look like this:

importPath: modded_fruit:deployment_graph

runtimeEnv: '{}'

host: 0.0.0.0

port: 8000

deployments:

- name: MangoStand
  userConfig: '{"price": 3, "fake_prop": "what_a_str!", "5": 2, "22": "hello"}'
  rayActorOptions:
    numCpus: 0.1

- name: OrangeStand
  userConfig: '{"price": 2, "fake_prop": "what_a_str!", "5": 2, "22": "hello"}'
  rayActorOptions:
    numCpus: 0.1

- name: PearStand
  userConfig: '{"price": 4, "fake_prop": "what_a_str!", "5": 2, "22": "hello"}'
  rayActorOptions:
    numCpus: 0.1

- name: FruitMarket
  numReplicas: 2
  rayActorOptions:
    numCpus: 0.1

- name: DAGDriver
  routePrefix: /

This is a bit more convoluted than e.g. this YAML spec where each key in the spec is listed on a newline with a pipe (|) character:

- name: OrangeStand
        numReplicas: 1
        userConfig: |
          price: 2
        rayActorOptions:
          numCpus: 0.1

Note: I anticipated that the JSON-serialized version would look worse than it is. Now that I've tried it, I actually don't think it looks too bad. I'd prefer to JSON-serialize and remove the manual step. What do you think @simon-mo @sihanwang41?

Copy link
Contributor

Choose a reason for hiding this comment

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

The output looks good to me. Without manual step, more convenient and less error-prone

Copy link
Contributor Author

@shrekris-anyscale shrekris-anyscale Oct 4, 2022

Choose a reason for hiding this comment

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

I've updated the code to JSON-serialize the fields that Kubernetes config expects to be strings:

  • runtime_env
  • deployment > user_config
  • deployment > ray_actor_options > runtime_env
  • deployment > ray_actor_options > resources

def snake_to_camel_case(snake_str: str) -> str:
"""Convert a snake case string to camel case."""

words = snake_str.lstrip("_").strip("_").split("_")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
words = snake_str.lstrip("_").strip("_").split("_")
words = snake_str.strip("_").split("_")

strip does lstrip's job already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for letting me know. I removed the lstrip call.

def dict_keys_snake_to_camel_case(snake_dict: dict) -> dict:
"""Converts dictionary's keys from snake case to camel case.

Does not modify original dictionary.
Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't recursive right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's not recursive.

@@ -478,6 +487,24 @@ def build(import_path: str, app_dir: str, output_path: Optional[str]):
).dict(exclude_defaults=True)
)

if kubernetes_format:
print(
"NOTE: Kubernetes requires the following fields to be "
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just json serialize it for the user instead?

Copy link
Contributor

@simon-mo simon-mo left a comment

Choose a reason for hiding this comment

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

Please add a note into the documentation as well.

Comment on lines +484 to +485
if kubernetes_format:
config = schema.kubernetes_dict(exclude_unset=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

quick non-blocking question here..
why do we have kubernetes_dict calls dict instead of calling dict first, then have a pure function that transform the config to k8s format? (basically why a method while the stuff is pretty pure functional)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question– the Kubernetes dictionary format had a few rules that were schema-specific:

  • All the keys in the config (including deployment dictionaries and ray_actor_options) should be camel-cased.
  • Some of the dictionaries need to be json-serialized:
    • runtime_env
    • deployment_dictionary > user_config
    • deployment dictionary > ray_actor_options > runtime_env
    • deployment_dictionary > ray_actor_options > resources)

I made it a method because the second requirement made it feel too specific to be a pure function.

Signed-off-by: Shreyas Krishnaswamy <[email protected]>
@simon-mo simon-mo merged commit 05e623e into ray-project:master Oct 6, 2022
maxpumperla pushed a commit that referenced this pull request Oct 7, 2022
WeichenXu123 pushed a commit to WeichenXu123/ray that referenced this pull request Dec 19, 2022
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.

[Serve] serve build option to output Kubernetes config format
3 participants