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

[Spot] Refactor spot APIs into spot.xxx #3417

Merged
merged 14 commits into from
Apr 11, 2024
Merged

[Spot] Refactor spot APIs into spot.xxx #3417

merged 14 commits into from
Apr 11, 2024

Conversation

Michaelvll
Copy link
Collaborator

@Michaelvll Michaelvll commented Apr 4, 2024

This PR refactors spot APIs core.spot_xxx into spot.xxx.

Closes #3283

Blocking #3419

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 (except test_spot_tpu, test_tpu_vm_pod, and tpu_vm_app due to [Examples] TPU example fail examples/tpu/tpuvm_mnist.yaml #3425)
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

@Michaelvll Michaelvll requested a review from cblmemo April 5, 2024 05:28
Copy link
Collaborator

@cblmemo cblmemo left a comment

Choose a reason for hiding this comment

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

Thanks! It mostly looks great to me except for some minor suggestions and a bug when calling sky.start instead of sky/core.py::_start; please reference to the comment above ;)

sky/spot/core.py Outdated Show resolved Hide resolved
sky/spot/core.py Show resolved Hide resolved
sky/__init__.py Outdated Show resolved Hide resolved
sky/spot/core.py Outdated Show resolved Hide resolved
sky/spot/core.py Outdated Show resolved Hide resolved
sky/spot/core.py Outdated Show resolved Hide resolved
sky/utils/dag_utils.py Outdated Show resolved Hide resolved
sky/utils/dag_utils.py Outdated Show resolved Hide resolved
""".strip()


def convert_entrypoint_to_dag(entrypoint: Any) -> 'dag_lib.Dag':
Copy link
Collaborator

Choose a reason for hiding this comment

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

code gardening: should we add proper type annotation for entrypoint

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For entrypoint, we check all the other types in the function below. It might be fine for now?

sky/utils/dag_utils.py Outdated Show resolved Hide resolved
@Michaelvll
Copy link
Collaborator Author

Michaelvll commented Apr 8, 2024

Tested:

@Michaelvll Michaelvll requested a review from cblmemo April 8, 2024 22:48
Copy link
Collaborator

@cblmemo cblmemo left a comment

Choose a reason for hiding this comment

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

Thanks! Looks great to me.

sky/spot/core.py Outdated Show resolved Hide resolved
@@ -626,3 +626,20 @@ def fill_template(template_name: str, variables: Dict,
content = j2_template.render(**variables)
with open(output_path, 'w', encoding='utf-8') as fout:
fout.write(content)


def deprecated_function(func: Callable, name: str, deprecated_name: str,
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there other deprecated function in the system? if so, change to this decorator as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, I don't think there is any other deprecated functions in the system.

@Michaelvll Michaelvll merged commit e60eb73 into master Apr 11, 2024
20 checks passed
@Michaelvll Michaelvll deleted the spot-refactor branch April 11, 2024 14:55
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.

[Spot] Move the managed spot related APIs under sky.spot
2 participants