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

Add Joblib Launcher for parallel execution (multi-process and multi-threaded) #392

Merged
merged 53 commits into from Feb 12, 2020
Merged

Add Joblib Launcher for parallel execution (multi-process and multi-threaded) #392

merged 53 commits into from Feb 12, 2020

Conversation

jan-matthis
Copy link
Contributor

@jan-matthis jan-matthis commented Jan 31, 2020

Motivation

Launcher for local parallel sweeps using Joblib.Parallel, as proposed in #357, and discussed with @omry.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Added tests following the example_launcher_plugin.

Related Issues and PRs

Implements #357

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 31, 2020
@omry
Copy link
Collaborator

omry commented Jan 31, 2020

Awesome!

Let's call this jobid_launcher.
Tell me when you are ready for code review.

@jan-matthis jan-matthis changed the title Add Parallel Launcher - WIP Add Joblib Launcher - WIP Feb 2, 2020
@jan-matthis
Copy link
Contributor Author

PR is ready for code review

As far as I can tell the CI failures should be unrelated to the plugin (?)

Copy link
Collaborator

@omry omry left a comment

Choose a reason for hiding this comment

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

Awesome! first round of review.

  1. rebase on master, it should address the omegaconf related errors.
  2. Address lint errors (nox -s lint is the easiest for now).
  3. Add a news fragment
  4. See my comments.

If you haven't already go through the contributing page in the website.

plugins/joblib_launcher/README.md Outdated Show resolved Hide resolved
plugins/joblib_launcher/README.md Outdated Show resolved Hide resolved
plugins/joblib_launcher/README.md Outdated Show resolved Hide resolved
plugins/joblib_launcher/README.md Outdated Show resolved Hide resolved
plugins/joblib_launcher/example/my_app.py Outdated Show resolved Hide resolved
@omry
Copy link
Collaborator

omry commented Feb 3, 2020

About the sort lint failure, I saw the same with the ax plugin.
try to add jobid to the third party projects listed in isort.cfg.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 5, 2020

This pull request introduces 1 alert when merging 81a2616 into e19b96b - view on LGTM.com

new alerts:

  • 1 for Module imports itself

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 5, 2020

This pull request introduces 1 alert when merging 61a1f9a into e19b96b - view on LGTM.com

new alerts:

  • 1 for Module imports itself

[__main__][INFO] - Process ID 14335 executing task 4 ...
[__main__][INFO] - Process ID 14337 executing task 5 ...
```
See [website](https://hydra.cc/docs/plugins/joblib_launcher) for more information
Copy link
Collaborator

Choose a reason for hiding this comment

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

Until 1.0.0 is released, the url will be:

https://hydra.cc/docs/next/plugins/joblib_launcher

website/versioned_sidebars/version-0.11-sidebars.json Outdated Show resolved Hide resolved
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 12, 2020

This pull request introduces 1 alert when merging 74eb284 into 0bdf355 - view on LGTM.com

new alerts:

  • 1 for Module imports itself

@omry
Copy link
Collaborator

omry commented Feb 12, 2020

  1. Run isort . at the root (and add the pre-commit hooks, see development).
  2. Please rebase on top of master to address the previous failures (from code coverage going down).

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 12, 2020

This pull request introduces 1 alert when merging 96744ec into 9f2cd9f - view on LGTM.com

new alerts:

  • 1 for Module imports itself

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 12, 2020

This pull request introduces 1 alert when merging d86c242 into 9f2cd9f - view on LGTM.com

new alerts:

  • 1 for Module imports itself

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 12, 2020

This pull request introduces 1 alert when merging 3b9cc8a into 9f2cd9f - view on LGTM.com

new alerts:

  • 1 for Module imports itself

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 12, 2020

This pull request introduces 1 alert when merging 1d10c9b into 9f2cd9f - view on LGTM.com

new alerts:

  • 1 for Module imports itself

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 12, 2020

This pull request introduces 1 alert when merging febcf68 into 9f2cd9f - view on LGTM.com

new alerts:

  • 1 for Module imports itself

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 12, 2020

This pull request introduces 1 alert when merging 4a421f7 into 9f2cd9f - view on LGTM.com

new alerts:

  • 1 for Module imports itself

Supresses error:
Untyped decorator makes function "test_discovery" untyped
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 12, 2020

This pull request introduces 1 alert when merging 9f92a1c into 9f2cd9f - view on LGTM.com

new alerts:

  • 1 for Module imports itself

omry and others added 3 commits February 12, 2020 14:46
Using a different circleci python 3.6 Image for Linux to work around new issue
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 12, 2020

This pull request introduces 1 alert when merging 7f2e75a into 9f2cd9f - view on LGTM.com

new alerts:

  • 1 for Module imports itself

@omry
Copy link
Collaborator

omry commented Feb 12, 2020

Because you are cherry-picking from master it's hard to say what this diff is adding and what is new.
Can you rebase instead of cherry picking?

.circleci/config.yml Show resolved Hide resolved
This example demonstrates how to to customize the logging behavior of your Hydra app, by making the following changes

This example demonstrates how to customize the logging behavior of your Hydra app, by making the following changes
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice catch.

@omry
Copy link
Collaborator

omry commented Feb 12, 2020

And btw: awesome that tests are now passing! :)

@omry omry merged commit 4a81bce into facebookresearch:master Feb 12, 2020
@omry
Copy link
Collaborator

omry commented Feb 12, 2020

This is awesome work, thanks for putting so much time into it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants