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] CLI args for task option and service config override for sky serve up #2879

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

cblmemo
Copy link
Collaborator

@cblmemo cblmemo commented Dec 16, 2023

This PR adds task option and service config override for sky serve up.

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
    • sky serve up 'python -m http.server 8080' --readiness-path / --initial-delay-seconds 120 --min-replicas 1 --ports 8080 -n test-cli-service
  • 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

@concretevitamin
Copy link
Member

A user bumped for this wanting --env.

sky/cli.py Outdated Show resolved Hide resolved
sky/cli.py Outdated Show resolved Hide resolved
sky/cli.py Show resolved Hide resolved
else:
override_params['cloud'] = clouds.CLOUD_REGISTRY.from_str(cloud)
resources_override_params['cloud'] = (
clouds.CLOUD_REGISTRY.from_str(cloud))
if region is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am wondering if we can refactor out this common pattern

    if field is not None:
         if field.lower() == 'none':
             resources_override_params['field_str'] = None
         else:
             resources_override_params['field_str'] = field

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a little bit tricky since some fields like clouds need special handling; Lets do this in another PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

sg.

Copy link
Collaborator

@MaoZiming MaoZiming left a comment

Choose a reason for hiding this comment

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

_make_task_or_dag_from_entrypoint_with_overrides a bit confusing here where the services overrides do not apply to dag with multiple tasks.

@cblmemo
Copy link
Collaborator Author

cblmemo commented Jan 10, 2024

_make_task_or_dag_from_entrypoint_with_overrides a bit confusing here where the services overrides do not apply to dag with multiple tasks.

Thanks for pointing it out! Actually, we don't need to do this check since skyserve doesn't support chain dag; ref

skypilot/sky/cli.py

Lines 4283 to 4285 in 6fd5bf2

if isinstance(task, sky.Dag):
raise click.UsageError(
_DAG_NOT_SUPPORTED_MESSAGE.format(command='sky serve up'))

Just removed the check.

Copy link
Collaborator

@MaoZiming MaoZiming 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 the quick fix. Please do more manual tests before merging : )

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 CLI arguments for sky serve @cblmemo! Left a comment below.

Comment on lines +1121 to +1128
readiness_path: Optional[str] = None,
initial_delay_seconds: Optional[int] = None,
min_replicas: Optional[int] = None,
max_replicas: Optional[int] = None,
target_qps_per_replica: Optional[float] = None,
post_data: Optional[str] = None,
upscale_delay_seconds: Optional[int] = None,
downscale_delay_seconds: Optional[int] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not exactly sure about having these service related arguments in the CLI as they might be a bit too detailed for a service. Can we leave them out for now (e.g., keep them in another branch), and only merge the CLI arguments for resources for sky serve CLI?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point! Done in #2979 and PTAL

Copy link
Contributor

This PR is stale because it has been open 120 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label May 24, 2024
@cblmemo cblmemo removed the Stale label May 24, 2024
Copy link
Contributor

This PR is stale because it has been open 120 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Sep 22, 2024
@cblmemo cblmemo removed the Stale label Sep 23, 2024
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.

4 participants