Skip to content

[MRG] Backend hints and shared memory constraints#595

Closed
ogrisel wants to merge 12 commits intojoblib:masterfrom
ogrisel:backend-hints
Closed

[MRG] Backend hints and shared memory constraints#595
ogrisel wants to merge 12 commits intojoblib:masterfrom
ogrisel:backend-hints

Conversation

@ogrisel
Copy link
Copy Markdown
Contributor

@ogrisel ogrisel commented Jan 22, 2018

This is an alternative implementation of #537. I reimplemented it from scratch because joblib had diverged a bit and I did not agree with the semantics of constraints violations in #537.

TODO:

  • test by running RandomForestClassifier on dask-distributed cluster and check that fit run trees in parallel (done with this branch of sklearn: scikit-learn/scikit-learn@master...ogrisel:joblib-backend-hints);
  • decide whether we should use prefer='threads' and require='sharedmem' or keep the currently implemented boolean flags for hinting and hard constraints;
  • update Parallel docstring to document the new options.

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 22, 2018

Codecov Report

Merging #595 into master will increase coverage by 0.12%.
The diff coverage is 96.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #595      +/-   ##
==========================================
+ Coverage    94.9%   95.03%   +0.12%     
==========================================
  Files          38       38              
  Lines        5005     5094      +89     
==========================================
+ Hits         4750     4841      +91     
+ Misses        255      253       -2
Impacted Files Coverage Δ
joblib/_parallel_backends.py 95.25% <100%> (+1.39%) ⬆️
joblib/parallel.py 98.68% <100%> (+0.1%) ⬆️
joblib/test/test_parallel.py 96.18% <93.65%> (+0.28%) ⬆️
joblib/memory.py 93.39% <0%> (-0.49%) ⬇️
joblib/test/test_memory.py 98.94% <0%> (-0.43%) ⬇️
joblib/disk.py 88.33% <0%> (+6.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c019f26...7ee44b9. Read the comment docs.

@ogrisel
Copy link
Copy Markdown
Contributor Author

ogrisel commented Jan 22, 2018

Actually, have require='sharedmem' is better because in the future we might want to use it for node tags for instance: require=['gpu', 'largemem'].

To make this consistent I will also rewrite this to use the prefer='threads' pattern.

if require == 'sharedmem' and not supports_sharedmem:
# This backend does not match the shared memory constraint:
# fallback to the default thead-based backend.
backend = BACKENDS[DEFAULT_THREAD_BACKEND]()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Would it be possible to add a log message here, and maybe line 94, saying that the backend was changed?

That would address @stephen-hoover's comment in #537 (comment)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@ogrisel ogrisel changed the title Backend hints and shared memory constraints [MRG] Backend hints and shared memory constraints Jan 23, 2018
@ogrisel
Copy link
Copy Markdown
Contributor Author

ogrisel commented Jan 24, 2018

@jcrist @stephen-hoover @TomAugspurger @mrocklin @GaelVaroquaux I think this is ready for review.

@ogrisel ogrisel closed this Jan 24, 2018
@ogrisel ogrisel deleted the backend-hints branch January 24, 2018 14:53
@ogrisel
Copy link
Copy Markdown
Contributor Author

ogrisel commented Jan 24, 2018

I messed up with git. I reopened the PR as #602.

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.

2 participants