Skip to content
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

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Worker ID functionality #261

wants to merge 15 commits into from

Conversation

pmla
Copy link

@pmla pmla commented Jul 3, 2020

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.

Copy link
Collaborator

@tomMoral tomMoral left a 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.

loky/process_executor.py Outdated Show resolved Hide resolved
loky/process_executor.py Outdated Show resolved Hide resolved
loky/process_executor.py Outdated Show resolved Hide resolved
loky/process_executor.py Outdated Show resolved Hide resolved
tests/test_worker_id.py Outdated Show resolved Hide resolved
tests/test_worker_id.py Show resolved Hide resolved
Copy link
Collaborator

@tomMoral tomMoral left a 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.

loky/process_executor.py Outdated Show resolved Hide resolved
loky/worker_id.py Outdated Show resolved Hide resolved
loky/worker_id.py Outdated Show resolved Hide resolved
tests/test_worker_id.py Outdated Show resolved Hide resolved
@danielyan86129
Copy link

Any progress? Very useful feature!

@scottgigante-immunai
Copy link

Would love to see this merged!

@qianyizhang
Copy link

any progress of getting merged??

@tomMoral
Copy link
Collaborator

tomMoral commented Apr 9, 2024

Thanks for the ping on this feature.
To make progress, the best would be to have people testing this feature extensively to see that it does not break.
One would need to rebase this one and see how it goes.

@tomMoral
Copy link
Collaborator

tomMoral commented Apr 9, 2024

Thanks for the ping on this feature.
To make progress, the best would be to have people testing this feature extensively to see that it does not break.
I can try to rebase this one and see how it goes.

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).

@qianyizhang
Copy link

Thanks for the ping on this feature. To make progress, the best would be to have people testing this feature extensively to see that it does not break. I can try to rebase this one and see how it goes.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants