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] Implement serve.build #23232

Merged
merged 122 commits into from
Mar 25, 2022
Merged

[serve] Implement serve.build #23232

merged 122 commits into from
Mar 25, 2022

Conversation

edoakes
Copy link
Contributor

@edoakes edoakes commented Mar 16, 2022

Why are these changes needed?

The Serve REST API relies on YAML config files to specify and deploy deployments. This change introduces serve.build() and serve build, which translate Pipelines to YAML files.

Related issue number

Closes #23137, closes #23138, and closes #23139.

Checks

  • 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 tests to assess the built YAML files' and Applications' behavior.


parameter_group.append(deployment_parameters)

internal_get_global_client().deploy_group(parameter_group, _blocking=blocking)
Copy link
Member

Choose a reason for hiding this comment

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

does deploy_group return a RayServeHandle as func signature says ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, not it doesn't. However, it also doesn't look like anything is using the public deploy_group, so I'll remove it.

python/ray/serve/drivers.py Outdated Show resolved Hide resolved
python/ray/serve/drivers.py Outdated Show resolved Hide resolved
Copy link
Member

@jiaodong jiaodong left a comment

Choose a reason for hiding this comment

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

dag part looks good to me, only nits.

python/ray/serve/tests/test_cli.py Outdated Show resolved Hide resolved
with open(output_path, "w") as f:
app.to_yaml(f)
else:
print(app.to_yaml(), end="")
Copy link
Member

Choose a reason for hiding this comment

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

not for this PR, but as a follow up it will be much more actionable to print other instructions for next step, such as providing a file path, or calling other clis to deployment the yaml file.

python/ray/serve/tests/test_pipeline_dag.py Outdated Show resolved Hide resolved
Copy link
Member

@jiaodong jiaodong left a comment

Choose a reason for hiding this comment

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

lg, i noticed last green commit has 500+ added lines but after lastest commit it's 338, did any file got untracked ?

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.

LGTM feel free to ignore my comment but would be a good check

attr_name = full_path[last_period_idx + 1 :]
module_name = full_path[:last_period_idx]

if ":" in full_path:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: check only one ":" exists?

Copy link
Contributor

Choose a reason for hiding this comment

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

I added a conditional to handle this.

@edoakes edoakes merged commit cf7b4e6 into ray-project:master Mar 25, 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
4 participants