-
Notifications
You must be signed in to change notification settings - Fork 415
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
[Core] Fix type of job_id
when querying job status
#2541
Conversation
job_id
when querying job status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing the issue @cblmemo! Left several comments.
@@ -368,10 +368,15 @@ def get_statuses_payload(job_ids: List[Optional[int]]) -> str: | |||
|
|||
def load_statuses_payload( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some docstr for this function to list all the possible cases for the dict? For example:
{None: None} there is no job on the cluster
{1: RUNNING, 2: None} job 2 does not exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is already included in the docstring of core.py::job_status
. I added a reference to it 🫡
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix @cblmemo! LGTM.
Co-authored-by: Zhanghao Wu <zhanghao.wu@outlook.com>
* fix * apply suggestions from code review * Apply suggestions from code review Co-authored-by: Zhanghao Wu <zhanghao.wu@outlook.com> * lint --------- Co-authored-by: Zhanghao Wu <zhanghao.wu@outlook.com>
Previously, our
common_utils.encode_payload
usedjson.dumps
to handle a dictionary with integer andNone
type as key, which will not be restored as the original type in defaultjson.loads
implementation; also, our click treatjob_id
as string, causing a lot of problems. This PR fixes these problems.Tested (run the relevant ones):
bash format.sh
sky logs --status cluster-with-no-job-submitted
sky logs --status cluster-with-job
pytest tests/test_smoke.py
pytest tests/test_smoke.py::test_fill_in_the_name
bash tests/backward_comaptibility_tests.sh