[Core] Return job_id and handle for sky/execution.py::_execute
#2736
Conversation
There was a problem hiding this comment.
Thanks for adding the return value to the APIs @cblmemo! Left several comments.
| @@ -370,6 +383,7 @@ def _execute( | |||
| subprocess_utils.run('sky status --no-show-spot-jobs', env=env) | |||
| print() | |||
| print('\x1b[?25h', end='') # Show cursor. | |||
| return job_id, handle | |||
There was a problem hiding this comment.
Seems we don't add a return to launch and exec functions? Do we need to pump it?
Also, it would be good to add a test in the smoke tests : )
Co-authored-by: Zhanghao Wu <[email protected]>
…t into execute-return-info
|
@Michaelvll Do you have time to look at this? It is required by the sky serve PR 👀 |
There was a problem hiding this comment.
Thanks for the update @cblmemo! Mostly loosk good to me. The only problem is with the tests.
tests/test_smoke.py
Outdated
| code = [ | ||
| f'name = "{name}"', | ||
| 'task = sky.Task(run="whoami")', | ||
| 'task.set_resources(sky.Resources(cloud=sky.GCP()))', | ||
| 'job_id, handle = sky.launch(task, cluster_name=name)', | ||
| 'assert job_id == 1', | ||
| 'assert handle is not None', | ||
| 'assert handle.cluster_name == name', | ||
| 'assert handle.launched_resources.cloud.is_same_cloud(sky.GCP())', | ||
| 'job_id_exec, handle_exec = sky.exec(task, cluster_name=name)', | ||
| 'assert job_id_exec == 2', | ||
| 'assert handle_exec is not None', | ||
| 'assert handle_exec.cluster_name == name', | ||
| 'assert handle_exec.launched_resources.cloud.is_same_cloud(sky.GCP())', | ||
| # For dummy task (i.e. task.run is None), the job won't be submitted. | ||
| 'dummy_task = sky.Task()', | ||
| 'job_id_dummy, _ = sky.exec(dummy_task, cluster_name=name)', | ||
| 'assert job_id_dummy is None', | ||
| ] |
There was a problem hiding this comment.
Instead of generating the python code, should we just have them inline?
There was a problem hiding this comment.
Wdym by 'inline'? Did you mean the following?
f"""
name = "{name}"
task = sky.Task(run="whoami")
task.set_resources(sky.Resources(cloud=sky.GCP()))
...
"""There was a problem hiding this comment.
No, I mean just write the python script, and let it be part of the test function instead of using generated code.
This PR added return value for
sky/execution.py::_execute.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