-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat(info): add missing Dask, engine and session information #164
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #164 +/- ##
==========================================
+ Coverage 84.18% 84.40% +0.21%
==========================================
Files 41 41
Lines 3542 3591 +49
==========================================
+ Hits 2982 3031 +49
Misses 471 471
Partials 89 89
|
1ee23ae
to
49656dc
Compare
49656dc
to
421f6bd
Compare
421f6bd
to
0a5c51a
Compare
0a5c51a
to
62333bd
Compare
if p.DaskClusterDefaultNumberOfWorkers != nil { | ||
displayInfoStringItem(cmd, p.DaskClusterDefaultNumberOfWorkers.Title, &p.DaskClusterDefaultNumberOfWorkers.Value) | ||
} | ||
if p.DaskClusterMaxMemoryLimit != nil { |
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.
You may want to amend the tests in order to have a case coverage for the new info
endpoint replies? Right new we have:
- testdata/inputs/info_big.json
- testdata/inputs/info_empty_inactivity_period.json
- testdata/inputs/info_small.json
You could introduce the workfow engine ones into "big" example, and you could introduce a new info_dask.json
example for Dask variables, since the new Dask feature is optional...
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.
question: The textual output of the Python client and the Go client is the same, however the order of outputted information differs, for example:
$ rc info | tail -1
Yadage engine version: 0.20.1
$ rcg info | tail -1
The maximum number of workers that users can ask for the single Dask cluster: 20
Do we want to harmonise this at this time?
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.
question: The JSON output of the Python client and the Go client differ with respect to null values:
$ rc info --json | jq > /tmp/z_py_j
$ rcg info --json | jq > /tmp/z_go_j
$ diff -u /tmp/z_py_j /tmp/z_go_j
--- /tmp/z_py_j 2025-01-21 13:46:00.209607732 +0100
+++ /tmp/z_go_j 2025-01-21 13:46:04.665618517 +0100
@@ -64,20 +64,17 @@
"value": "False"
},
"kubernetes_max_memory_limit": {
- "title": "Maximum allowed memory limit for Kubernetes jobs",
- "value": null
+ "title": "Maximum allowed memory limit for Kubernetes jobs"
},
"maximum_interactive_session_inactivity_period": {
- "title": "Maximum inactivity period in days before automatic closure of interactive sessions",
- "value": null
+ "title": "Maximum inactivity period in days before automatic closure of interactive sessions"
},
"maximum_kubernetes_jobs_timeout": {
"title": "Maximum timeout for Kubernetes jobs",
"value": "1209600"
},
"maximum_workspace_retention_period": {
- "title": "Maximum retention period in days for workspace files",
- "value": null
+ "title": "Maximum retention period in days for workspace files"
},
"snakemake_engine_version": {
"title": "Snakemake engine version",
Do we want to address this (admittedly minor) difference?
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.
todo: For the commit log message, I would simply use
feat(info): print information about Dask and workflow engines (#164)
Motivation: the "missing" is from the point of view of compatibility with the Python client, but it's just a matter of time that that one was implemented first. It could have been the other way round... So from the point of view of the release notes of the go client, this item was not really "missing" before, because we did not have any Dask at the time yet. Hence it's nicer to just say that this was added, but not that it was "missing".
62333bd
to
ef6c3b9
Compare
ef6c3b9
to
b7e8357
Compare
Closes #163