-
Notifications
You must be signed in to change notification settings - Fork 39
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(dask): add nthreads option to dask-worker
command
#636
Conversation
dask-worker
command (#636)dask-worker
command
@@ -346,6 +346,11 @@ def _parse_interactive_sessions_environments(env_var): | |||
) | |||
"""Maximum memory for one Dask worker.""" | |||
|
|||
REANA_DASK_CLUSTER_DEFAULT_SINGLE_WORKER_THREADS = int( |
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: We may need to sanitize the inputs. E.g. I have passed a non-integer value:
dask:
image: docker.io/coffeateam/coffea-dask-cc7:0.7.22-py3.10-g7f049
single_worker_number_of_threads: 4.5
and the workflow was accepted for execution. (Perhaps we could verify integer-ness of the value already in the client, as the first line of defense. But we should also be sanitising on the server-side as well.)
The pod was obviously not starting due to:
$ k logs reana-run-dask-916d3266-default-worker-aa138e6474-5dc87954ngqzx
Usage: dask-worker [OPTIONS] [SCHEDULER] [PRELOAD_ARGV]...
Try 'dask-worker --help' for help.
Error: Invalid value for '--nthreads': '4.5' is not a valid integer.
And the pod went into a crash loop:
$ k get pods | grep -E '(NAME|aa13)'
NAME READY STATUS RESTARTS AGE
reana-run-dask-916d3266-default-worker-aa138e6474-5dc87954ngqzx 0/1 CrashLoopBackOff 5 (2m12s ago) 4m58s
And the user would see the workflow as running:
$ rcg list
NAME RUN_NUMBER CREATED STARTED ENDED STATUS
threads-45 1 2025-02-20T13:13:10 2025-02-20T13:13:22 - running
Dunno when it would eventually stop, but we should exit early in these error situations.
Can you please round to the nearest integer on the server-side? And, whilst fixing this concrete usage scenario, can you please check also similar bordeline situations when a user passes an invalid container image or an invalid memory value?
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.
I added a check in reana-commons to prevent users from providing non-integer values. Since the OpenAPI specification in reana_analysis_schema.json
already allows validation through type and regex, and considering they are enough to cover the basic checks, I guess it is sufficient, and we may not need additional checks beyond the initial reana.yaml validation.
Following the integration of Dask into REANA and discussions with several REANA users, we identified the need to make the number of threads configurable. In this commit, we propagate the specified number of threads per worker or the default value to the `dask-worker` command. Closes reanahub/reana#874
2f28672
to
4a8b871
Compare
Following the integration of Dask into REANA and discussions with several REANA users, we identified the need to make the number of threads configurable. In this commit, we propagate the specified number of threads per worker or the default value to the `dask-worker` command. Closes reanahub/reana#874
4a8b871
to
95eb563
Compare
Following the integration of Dask into REANA and discussions with several REANA users, we identified the need to make the number of threads configurable. In this commit, we propagate the specified number of threads per worker or the default value to the `dask-worker` command. Closes reanahub/reana#874
95eb563
to
4395a3d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #636 +/- ##
==========================================
+ Coverage 73.16% 73.20% +0.04%
==========================================
Files 17 17
Lines 1986 1989 +3
==========================================
+ Hits 1453 1456 +3
Misses 533 533
|
Following the integration of Dask into REANA and discussions with several REANA users, we identified the need to make the number of threads configurable. In this commit, we propagate the specified number of threads per worker or the default value to the `dask-worker` command. Closes reanahub/reana#874
4395a3d
to
eaf2b59
Compare
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.
Works nicely 👍
Following the integration of Dask into REANA and discussions with several REANA users, we identified the need to make the number of threads configurable.
In this commit, we propagate the specified number of threads per worker or the default value to the
dask-worker
command.Closes reanahub/reana#874