8000 [MRG] Automatically group short tasks in batches by ogrisel · Pull Request #157 · joblib/joblib · GitHub
[go: up one dir, main page]

Skip to content

[MRG] Automatically group short tasks in batches #157

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

Merged
merged 9 commits into from
Jun 29, 2015

Conversation

ogrisel
Copy link
Contributor
@ogrisel ogrisel commented Jul 20, 2014

This is based on an online evaluation of the batch completion time to dynamically tune the size of the batch of tasks to dispatch at once to a worker.

TODO:

  • ensure tests pass on windows
  • update the change log
  • test with scikit-learn

@vene
Copy link
Contributor
vene commented Jul 20, 2014

One of the applications that brought this to attention comes from the parallel OvR metaestimator, where some benchmarks revealed the estimation being much faster without parallelization if the number of classes (and therefore parallel jobs) is very high (4000).

Benching script

The following is with default settings except where specified

With old joblib:

n_jobs=-1
{'dataset': 'sparse_output.joblib'}
Time to fit     = 393.623631001
Time to predict = 29.2175137997

sparse, n_jobs=1
Time to fit     = 35.092443943
Time to predict = 28.9341239929

With this PR:

n_jobs=-1
Time to fit     = 14.2408790588
Time to predict = 29.649130106

This seems great!

@ogrisel
Copy link
Contributor Author
ogrisel commented Jul 20, 2014

All tests under windows pass: https://ci.appveyor.com/project/ogrisel/joblib/build/1.0.29

I had to push bench-batching in the upstream repo for appveyor to run the test for some reason, we can delete that branch once we merge.

@ogrisel ogrisel changed the title [WIP] Automatically group short tasks in batches [MRG] Automatically group short tasks in batches Jul 20, 2014
@ogrisel
Copy link
Contributor Author
ogrisel commented Jul 20, 2014

@GaelVaroquaux this is ready for final review. @larsmans you might also be interested in having a look at this.

@ogrisel
Copy link
Contributor Author
ogrisel commented Jul 22, 2014

@jnothman I think you might also be interested in reviewing this. There is a benchmark script you can use to explore the runtime behavior.

@@ -132,10 +158,10 @@ def delayed_function(*args, **kwargs):
class ImmediateApply(object):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the class name should be changed to 'ImmediateComputeBatch': the 'ImmediateApply' name comes from the 'apply' terminology used in multiprocessing.

The docstring should clearly be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

@jnothman
Copy link
Contributor

I'll have to see if I have time to review, but @vene's summary above is very enticing!

@arjoly
Copy link
arjoly commented Jul 22, 2014

note that the predict is not parallelized. :-)

@ogrisel
Copy link
Contributor Author
ogrisel commented Jul 22, 2014

I think the first batch of comments are addressed in new commits. I plan to squash those prior to merging once the review is over.

Default is '2*n_jobs'. When batch_size="auto" this is reasonable
default and the multiprocessing workers shoud never starve.
batch_size: int or 'auto', default: 'auto'
The number of atomic tasks to dispatch at once to a specific
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"a specific worker" isn't quite right here. Perhaps just "to each worker"?

@ogrisel
Copy link
Contributor Author
ogrisel commented Jul 27, 2014

@jnothman thanks for the review. I addressed your comments in the last commit.

@jnothman
Copy link
Contributor

I haven't reviewed the benchmark or the tests, but the feature itself looks great!

@lesteve
Copy link
Member
lesteve commented Jun 23, 2015

Looks like the memory_profiler profile decorators had a significant overhead in the benchmark scripts. So here are the more accurate timings:

Dense 0.8.4 PR 157
n_jobs=1 47.71 47.65
n_jobs=-1 125.49 20.45
Sparse 0.8.4 PR 157
n_jobs=1 40.13 40.60
n_jobs=-1 113.41 10.73

@ogrisel
Copy link
Contributor Author
ogrisel commented Jun 23, 2015

Thanks for the benchmarks!

@ogrisel
Copy link
Contributor Author
ogrisel commented Jun 23, 2015

BTW, you asked earlier I how to test the dev branch of joblib in scikit-learn without modifying the scikit-learn source code: I put a monkeypatch in a pythonstartup.py file that gets executed each time you start python:

$ cat ~/pythonstartup.py
try:
    import joblib
    from sklearn.externals import joblib as skl_joblib
    print('Monkeypatching scikit-learn embedded joblib')
    for k, v in vars(joblib).items():
        setattr(skl_joblib, k, v)
except ImportError as e:
    print("Could not apply joblib monkeypatch: %s" % e)



def test_batching_auto_multiprocessing():
# Batching is not enabled whith the threading backend as it has found
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this comment is a copy and paste of the one in test_batching_auto_threading and should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch

self.n_dispatched_batches += 1
self.n_dispatched_tasks += len(batch)
self.n_completed_tasks += len(batch)
if not _verbosity_filter(self.n_dispatched_batches, self.verbose):
self._print('Done %3i jobs | elapsed: %s',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You didn't change jobs to tasks here but you did it below somewhere.

@ogrisel
Copy link
Contributor Author
ogrisel commented Jun 25, 2015

@lesteve I pushed a fix for the batch_size=0 you found while running the benchmarks.

bench_short_tasks(low_variance, **bench_parameters)

# Second pair of benchmarks: one has a cycling task duration pattern that
# the auto batching feature should be able to roughly track. We use a pair
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by pair I assume you mean even? 🇫🇷

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no I mean a couple, e.g. like in the expression "a pair of gloves": https://www.google.fr/search?q=define:pair

Isn't that idiomatic English?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean "a pair power of cos". The first use of "pair" is perfectly readable to me, but the second struck as odd.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok, will fix it.

@vene
Copy link
Contributor
vene commented Jun 25, 2015

Thanks a lot @ogrisel for the work on this, is there anything I can do to help?

@ogrisel
Copy link
Contributor Author
ogrisel commented Jun 25, 2015

Thanks a lot @ogrisel for the work on this, is there anything I can do to help?

Thanks for the final review. I think we are good. I will let @lesteve do the merge and release joblib 0.9.0b1. Then I will do a PR in sklearn to synchronize the embedded joblib. That should get us some quick feedback.

@lesteve
Copy link
Member
lesteve commented Jun 29, 2015

Merging, thanks a lot, great stuff.

lesteve added a commit that referenced this pull request Jun 29, 2015
[MRG] Automatically group short tasks in batches
@lesteve lesteve merged commit aa29032 into joblib:master Jun 29, 2015
@jnothman
Copy link
Contributor

Yay!

On 29 June 2015 at 16:51, Loïc Estève notifications@github.com wrote:

Merged #157 #157.


Reply to this email directly or view it on GitHub
#157 (comment).

@arjoly
Copy link
arjoly commented Jun 29, 2015

Great !!!

@ogrisel
Copy link
Contributor Author
ogrisel commented Jun 29, 2015

🍻

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.

7 participants
0