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

replace @distributed with pmap #54

Open
OkonSamuel opened this issue Jun 9, 2020 · 5 comments
Open

replace @distributed with pmap #54

OkonSamuel opened this issue Jun 9, 2020 · 5 comments

Comments

@OkonSamuel
Copy link
Member

OkonSamuel commented Jun 9, 2020

Currently in MLJ acceleration with CPUThreads is implemented using @distributed. This effectively splits up the given range (1:nfolds or 1:nmetamodels) into equal chunks and sends them off to all workers loaded with addprocs. This is great if the each chunk runs in the same amount of time otherwise some overhead is experienced. Also the user lacks the ability to specify the actual workers to be used in computing. (This might not be a big deal)
pmap implementation allows user more control (if they wish) in how these tasks are sent to to these workers.(this is due to batch_size and AbstractWorkerPool options it exposes).
Previously the main reason for not adopting pmap was because nested pmap hangs see JuliaLang/Distributed.jl#62 (There is a workaround this stated there).
The only limitation left in adopting this is that calling pmap from within Threads.@spawn some times hangs.( Although i don't think it is practical to call pmap from threads. What is more common is calling threads from processes) see JuliaLang/Distributed.jl#69

@ablaom
Copy link
Member

ablaom commented Jun 10, 2020

@OkonSamuel Thanks for that explantation. It's good we are tracking this kind of issue.

It sounds like we might want to wait for the cited issues to be resolved before making any changes here, right?

@ablaom
Copy link
Member

ablaom commented Jun 10, 2020

Or are you saying we could safely implement the workaround (under the hood - the user doesn't need to do anything)?

@OkonSamuel
Copy link
Member Author

Or are you saying we could safely implement the workaround (under the hood - the user doesn't need to do anything)

Yes. With the only caveat being we won't be able to use processes from within threads.

@OkonSamuel
Copy link
Member Author

@ablaom. You may not like this. But i would suggest disabling the option of nesting CPUProcesses in CPUThreads until this finalizer issue is sorted out by julia.
reason being the build sometimes hangs with this option (and it's not because of ProgressMeter printing because the same occurs when verbosity is set as zero)
see here

@ablaom
Copy link
Member

ablaom commented Jun 11, 2020

No, no, that's sound like a reasonable suggestion, especially if you are happy to make the PR's 😄 There could be some tedium around updating the tests, which presently cycle through all possible combinations.

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

No branches or pull requests

2 participants