[Spot/UX] Print spot jobs' resources before confirmation #2524
Conversation
Michaelvll
changed the title
sky/task.py
Outdated
| self.estimated_inputs_size_gigabytes = None | ||
| self.estimated_outputs_size_gigabytes = None | ||
| self.estimated_inputs_size_gigabytes = 0.0 | ||
| self.estimated_outputs_size_gigabytes = 0.0 |
There was a problem hiding this comment.
Why changing these values?
There was a problem hiding this comment.
It is because we use the value in the optimizer, but not correctly handle None. I changed the handling in the Optimizer instead. Thanks for catching this.
Co-authored-by: Tian Xia <cblmemo@gmail.com>
…ypilot into print-resources-for-spot
| @@ -423,7 +424,7 @@ def from_yaml(yaml_path: str) -> 'Task': | |||
| ValueError: if the path gets loaded into a str instead of a dict; or | |||
| if there are any other parsing errors. | |||
| """ | |||
| with open(os.path.expanduser(yaml_path), 'r') as f: | |||
| with open(os.path.expanduser(yaml_path), 'r', encoding='utf-8') as f: | |||
There was a problem hiding this comment.
Why change encoding here?
There was a problem hiding this comment.
It is recommended to always specify the encoding in open function. We did not change the encoding, but just use the default utf-8. https://peps.python.org/pep-0597/
| @@ -44,7 +45,7 @@ | |||
| 'a list of node ip addresses (List[str]). Got {run_sig}') | |||
|
|
|||
|
|
|||
| def _is_valid_name(name: str) -> bool: | |||
| def _is_valid_name(name: Optional[str]) -> bool: | |||
There was a problem hiding this comment.
A little bit curious why it could pass mypy check previously... Maybe we should file an issue to add type notation for these task-related functions? A lof of function still doesn't have a notation, like estimate_runtime or get_resources.
There was a problem hiding this comment.
Yes, we should. Mypy will ignore the functions/variables without type annotation, so probably skip some callers with name that is None.
| self.estimated_inputs_size_gigabytes = None | ||
| self.estimated_outputs_size_gigabytes = None | ||
| self.inputs: Optional[str] = None | ||
| self.outputs: Optional[str] = None |
There was a problem hiding this comment.
Maybe type notation for set_outputs and get_outputs too? Since it is relevant to this PR
There was a problem hiding this comment.
Good call! Added. Thanks!
Before this PR launching a spot job will not show any information about the resources used by the spot job.
After this PR, the chosen resources for the spot job will be shown before confirmation.
Tested (run the relevant ones):
bash format.shpytest tests/test_smoke.pypytest tests/test_smoke.py::test_fill_in_the_namebash tests/backward_comaptibility_tests.sh