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

Update Dask backend #1411

Merged
merged 17 commits into from Apr 12, 2023
Merged

Update Dask backend #1411

merged 17 commits into from Apr 12, 2023

Conversation

scharlottej13
Copy link
Contributor

@scharlottej13 scharlottej13 commented Mar 27, 2023

Opening this PR to update the documentation for using the Dask backend, since it seems the last changes were in #613.

Noting this is also referenced in the scikit-learn docs. Here's a screenshot of the changed from building the sklearn docs locally:

Screen Shot 2023-03-27 at 3 53 55 PM

cc @mrocklin @ncclementi @jrbourbeau

@codecov
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.02 🎉

Comparison is base (5543c71) 94.80% compared to head (86df3af) 94.83%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1411      +/-   ##
==========================================
+ Coverage   94.80%   94.83%   +0.02%     
==========================================
  Files          45       45              
  Lines        6889     6889              
==========================================
+ Hits         6531     6533       +2     
+ Misses        358      356       -2     
Impacted Files Coverage Δ
joblib/parallel.py 96.02% <ø> (ø)

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@scharlottej13 scharlottej13 marked this pull request as ready for review March 28, 2023 00:04
@ncclementi
Copy link

This looks good to me, I wonder who we should ping for a review.
It seems like the ci failures are unrelated to this PR.

Maybe a gentle ping to @GaelVaroquaux or @tomMoral

@jrbourbeau jrbourbeau mentioned this pull request Mar 29, 2023
Copy link

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @scharlottej13 @ncclementi. I agree the failures in CI appear to be unrelated to the changes here. I'm checking in #1412 whether or not those failures are already present on the main branch

Also cc @ogrisel @lesteve in case either of you have bandwidth for a review

EDIT: The linting failure does look related

@jrbourbeau
Copy link

Okay, so the linting error is (clearly) related to the changes here. The linux_pypy3 build isn't failing in #1412, but also seems unrelated (at least to me naively). Maybe it's a transient error? @scharlottej13 would you mind merging the main branch and resolving that linting issue? That will trigger another CI build and we can see if the linux_pypy3 failure persists

@scharlottej13
Copy link
Contributor Author

Thanks @jrbourbeau and @ncclementi!

Okay, so the linting error is (clearly) related to the changes here.

Yes, it seems to be happening here due to a long link (line 10). Unfortunately I'm not quite sure what the best fix is. I tried separating the link from the target, but the link didn't render correctly in the RTD build. I also tried ignoring the line with # noqua: E501, however, then that comment shows up in the documentation too.

The linux_pypy3 build isn't failing in #1412, but also seems unrelated (at least to me naively). Maybe it's a transient error? @scharlottej13 would you mind merging the main branch and resolving that linting issue? That will trigger another CI build and we can see if the linux_pypy3 failure persists

This test seems to be passing now! My branch (and fork) were both already up to date with main.

Copy link
Member

@lesteve lesteve left a comment

Choose a reason for hiding this comment

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

This looks good.

Just curious, were they something in particular that prompted your PR, e.g. something that you thought was not up-to-date or missing?

Also, it would be nice to try to make the linter happy, I'll try to take a look (probably next week).

joblib/parallel.py Outdated Show resolved Hide resolved
@scharlottej13
Copy link
Contributor Author

Just curious, were they something in particular that prompted your PR, e.g. something that you thought was not up-to-date or missing?

Thanks for having a look @lesteve! Yes, the Deploy Dask Clusters was recently updated in the Dask docs (see dask/dask#9912, should have mentioned that in my initial comment).

Also, it would be nice to try to make the linter happy, I'll try to take a look (probably next week).

Appreciate your help with this.

Copy link
Contributor

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

Thx a lot for the PR.
a couple of comment but otherwise LGTM.

joblib/parallel.py Show resolved Hide resolved
joblib/parallel.py Outdated Show resolved Hide resolved
@scharlottej13
Copy link
Contributor Author

Thanks @lesteve and @tomMoral for the review! I believe I've responded to your comments. Feel free to let me know if there's anything else.

Copy link
Contributor

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

LGTM, just did a small reorder of the backend to put dark first as it is the only one with an example.
Let's change the docstring example in a follow up PR.

doc/parallel.rst Outdated Show resolved Hide resolved
joblib/parallel.py Show resolved Hide resolved
@tomMoral tomMoral merged commit 2d284d1 into joblib:master Apr 12, 2023
16 checks passed
@tomMoral
Copy link
Contributor

thanks @scharlottej13 for the PR :)

@scharlottej13
Copy link
Contributor Author

Of course, thanks @tomMoral!

@scharlottej13 scharlottej13 deleted the update-dask branch April 12, 2023 15:45
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.

None yet

5 participants