-
Notifications
You must be signed in to change notification settings - Fork 422
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
Update Dask backend #1411
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
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
... 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. |
This reverts commit 1cb2adf.
This looks good to me, I wonder who we should ping for a review. Maybe a gentle ping to @GaelVaroquaux or @tomMoral |
There was a problem hiding this 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
Okay, so the linting error is (clearly) related to the changes here. The |
Thanks @jrbourbeau and @ncclementi!
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
This test seems to be passing now! My branch (and fork) were both already up to date with main. |
There was a problem hiding this 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).
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).
Appreciate your help with this. |
There was a problem hiding this 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.
There was a problem hiding this 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.
thanks @scharlottej13 for the PR :) |
Of course, thanks @tomMoral! |
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:
cc @mrocklin @ncclementi @jrbourbeau