-
Notifications
You must be signed in to change notification settings - Fork 45
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
Worker ID functionality #261
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for the PR! I did a first pass with a bunch of comments.
I think the only thing that is not accounted for is the resize of the executor for smaller number of workers but I am not sure how reliable we can make this.
I think if it is too complex, we can have a go with this implem and document the fact that downsizing the size of the executor will leave you with unreliable worker_id.
One thing that would be great would be to add a docstring to get_worker_id
explaining the rational and how to use it.
Co-authored-by: Thomas Moreau <[email protected]>
Co-authored-by: Thomas Moreau <[email protected]>
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.
It would be nice to add a test where you make sure all ID in [0, max_worker)
are given in an executor. You can use jobs that do not return before an event is set such as in tests/_test_process_executor.py-540:test_timeout
.
Co-authored-by: Thomas Moreau <[email protected]>
Co-authored-by: Thomas Moreau <[email protected]>
Co-authored-by: Thomas Moreau <[email protected]>
Any progress? Very useful feature! |
Would love to see this merged! |
any progress of getting merged?? |
Thanks for the ping on this feature. |
Thanks for the ping on this feature. Bote that another implementation of this feature is in #285 (IIRC, I forgot this PR existed and give a try to make this feature, sorry for the duplication). |
Either implementation appears to be satisfactory, but I prefer this one as it introduces an environment variable rather than an attribute, which seems less complex. This feature does not alter any existing functionality, so please proceed with a release if feasible. |
Hello loky team.
I would like to implement a "worker ID" function for loky. I originally wrote an issue and a pull request on the joblib github page. @tomMoral has kindly pointed me to the correct place for this PR.
@tomMoral: I have stripped this PR down so that it is relevant to loky and added a unit test. The worker ID assignment now accounts for available IDs with the
process_worker_id
variable. I'm not sure if I have accounted for all mechanisms by which processes can be ended though.