-
Notifications
You must be signed in to change notification settings - Fork 27
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
🎨⚗️Comp backend/bugfix/work stealing (⚠️ devops) #5261
🎨⚗️Comp backend/bugfix/work stealing (⚠️ devops) #5261
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5261 +/- ##
=========================================
+ Coverage 81.9% 87.3% +5.4%
=========================================
Files 394 1302 +908
Lines 14367 53251 +38884
Branches 381 1167 +786
=========================================
+ Hits 11767 46491 +34724
- Misses 2528 6511 +3983
- Partials 72 249 +177
Flags with carried forward coverage won't be shown. Click here to find out more.
|
09ee122
to
ce3f9c1
Compare
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
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.
nice, thanks!
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.
Is the new ENV safe? Can we merge it immediately for all deployments?
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.
👍
@YuryHrytsuk : I am not sure why it would not be safe. it is an addition, so yes it is fully safe or am I missing something there? |
No worries. I am asking because I don't know |
What do these changes do?
As noted while testing, there are some cases where the dask-workers are not balancing the workload optimally.
As explained in the dask-distributed documentation, the queue of each worker may be adjusted by changing the number of threads a worker provides and/or the worker-saturation. Since the first one is easily changeable, this PR allows to do just that.
In a billable product where 1 machine can run 1 job it also does not really make sense to provide more than 1 thread per worker.
Therefore a new ENV is introduced at the clusters-keeper service level.
Bonus:
Related issue/s
How to test
Dev Checklist
DevOps Checklist
CLUSTERS_KEEPER_DASK_NTHREADS
(see https://git.speag.com/oSparc/osparc-ops-deployment-configuration/-/merge_requests/306)