-
Notifications
You must be signed in to change notification settings - Fork 1.3k
No daemonic with threads #1018
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
No daemonic with threads #1018
Conversation
Codecov Report
@@ Coverage Diff @@
## development #1018 +/- ##
===============================================
- Coverage 85.36% 85.32% -0.05%
===============================================
Files 125 125
Lines 9848 9857 +9
===============================================
+ Hits 8407 8410 +3
- Misses 1441 1447 +6
Continue to review full report at Codecov.
|
|
Great to see that this just works. I don't think we need to test for workers spawned with processes then (and you can also change the examples then) |
|
I think this is still needed for the examples, because when adding new workers, they are added as a new process. To scale on a local thread, one usually does the scale command of localcluster. Is it ok to leave it like this? or do you want to change the examples? |
Could you please try? If this is indeed the case, I think it would be good to add this as a comment to the example.
No, not with respect to that |
Yes, this is needed in this context. Removing it causes multiple crashes. I have updated the example comment. |
If moving to thread based dask workers, we do not need to concern ourselves with daemonic workers.
Below line can be removed. Nevertheless, the question remains if for our testing we should keep this, and for this I am talking about:
auto-sklearn/test/conftest.py
Line 167 in 13d1c06
Does it makes sense to still test with process==true for the user that wants to provide a process=true dask client?