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

JSON output for hq job list and hq submit #24

Merged
merged 2 commits into from
Jul 17, 2024

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Jul 17, 2024

fixes #13

Instead of using regex to parse the command output, HQ provide output mode JSON which we can get output to a dict to avoid using regex.

unkcpz and others added 2 commits July 17, 2024 14:25
Instead of using regex to parse the command output, HQ provide output
mode JSON which we can get output to a dict to avoid using regex.
@unkcpz
Copy link
Member Author

unkcpz commented Jul 17, 2024

@t-reents You may want to take a look and continue on #21?

@t-reents
Copy link
Contributor

Hi @unkcpz
You mean to also implement the parse_output method which builds on the detailed_job_info_command I implemented? I'm not 100% sure what you have in mind

@unkcpz unkcpz requested a review from t-reents July 17, 2024 14:43
@unkcpz
Copy link
Member Author

unkcpz commented Jul 17, 2024

Hi @t-reents, I just want to pin you as reviewer but you were not in the org so I am not able to make you the reviewer. BTW, this PR contains some change in test, it may help you with adding test to #21.

Copy link
Contributor

@t-reents t-reents left a comment

Choose a reason for hiding this comment

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

Just a small comment, apart from that it's fine

job_info = JobInfo()
job_info.job_id = hq_job_dict["id"]
job_info.title = hq_job_dict["name"]
stats: t.List[str] = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Mainly out of curiosity, why do you explicitly add a type hint here? I'm mainly thinking about consistency, as I didn't see it anywhere else in the plugin apart from the function definitions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to make my IDE happy, I am using neovim with ruff check, which is quite strict on typing. The type of elements in the list is deducted from the key of hq_job_dict["task_stats"].
By tell my LSP the type of element explicitly, stats[0] knows it is a str and help me to hint upper() is a valid method to call.

@t-reents
Copy link
Contributor

Yes, I'll consider it for the tests. Thanks a lot for including me in the precious aiidateam org 😄

@t-reents t-reents mentioned this pull request Jul 17, 2024
@unkcpz unkcpz merged commit d53617a into aiidateam:main Jul 17, 2024
3 checks passed
@unkcpz unkcpz deleted the fix/13/json-hq-command-output branch July 17, 2024 15:28
unkcpz added a commit to unkcpz/aiida-hyperqueue that referenced this pull request Jul 20, 2024
fixes aiidateam#29

The id field of JobInfo is expecting a str. In aiidateam#24 when parsing JSON
output of `hq job list` the json loads will use the int for the id
parsed directly. Wrong type causes the subtle issue that when job is
waiting it not get into QUEUED state, but immediatly finished and get
nothing to parse from output.
unkcpz added a commit that referenced this pull request Jul 22, 2024
fixes #29

The id field of JobInfo is expecting a str. In #24 when parsing JSON
output of `hq job list` the json loads will use the int for the id
parsed directly. Wrong type causes the subtle issue that when job is
waiting it not get into QUEUED state, but immediatly finished and get
nothing to parse from output.
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.

Switch to the new JSON output for parsing the output of HQ
2 participants