Gather CV Results as Completed #433

vecchp · 2018-12-01T08:09:54Z

In progress PR to address #415.

TomAugspurger · 2018-12-01T13:28:20Z

The CI failures can probably be ignored. I'll try to get them fixed next week.

@vecchp can you think of a way to test this? The best I've come up with is a custom estimator class, that when given a specific hyper parameter combination does something like

get_worker inside the task and closes the worker, to simulate it going away. https://distributed.dask.org/en/latest/api.html#distributed.get_worker. Haven't tested that out yet.

@jcrist will you have time next week to give feedback on this approach?

vecchp · 2018-12-02T07:54:59Z

@TomAugspurger I'll think a little about the tests, though your current suggestion seems reasonable.

… there a better way?

dask_ml/model_selection/_search.py

vecchp · 2018-12-05T07:20:36Z

@jcrist @TomAugspurger made some changes w.r.t the PR comments. On my end all tests pass except for test_cache_cv. I believe the failure is due to the cache being hit more than the previous expected values given the current changes.

build_graph is now broken into build_cv_graph & build_result_graph, I had to return main_token, X_name, y_name, weights, fit_params and then pass it to build_result_graph which seems a bit messy, but I haven't thought of a better way. What are your thoughts on these changes now?

I also haven't got around to confirming if the futures no longer need to be canceled or writing robust tests for the new as_completed feature.

…ying private attributes.

vecchp · 2019-04-18T17:00:33Z

@TomAugspurger checking in again to see what else should be done. All tests pass.

TomAugspurger · 2019-04-18T18:20:14Z

Sorry, I won't have time to look at this for at least the next couple weeks.

…

On Thu, Apr 18, 2019 at 12:00 PM Paul Vecchio ***@***.***> wrote: @TomAugspurger <https://github.com/TomAugspurger> checking in again to see what else should be done. All tests pass. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#433 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKAOITO6OQG7S65S3PORK3PRCSLFANCNFSM4GHTECOQ> .

TomAugspurger · 2019-04-18T18:22:20Z

All tests pass.

It seems like coverage is decreasing. Does that make sense, or is this a false positive? https://codecov.io/gh/dask/dask-ml/compare/cfa57e5789fc29bdcee5d846b5116c07f5fbe292...9da5b8f0cfa5960a7ee70f216a684bff96a450b2/changes

vecchp · 2019-05-04T16:51:52Z

All tests pass.

It seems like coverage is decreasing. Does that make sense, or is this a false positive? https://codecov.io/gh/dask/dask-ml/compare/cfa57e5789fc29bdcee5d846b5116c07f5fbe292...9da5b8f0cfa5960a7ee70f216a684bff96a450b2/changes

Makes sense, looks like it's dead code given the as_completed implementation no longer uses it. I'll update.

…results_as_completed

vecchp · 2019-05-04T17:34:38Z

@TomAugspurger looks like all stages pass. I think codecov was removed but that should be back to normal as well given the dead code is now gone.

stsievert · 2019-05-04T21:39:59Z

dask_ml/model_selection/_search.py

 try:
    from cytoolz import get, pluck
 except ImportError:  # pragma: no cover
    from toolz import get, pluck

+try:
+    from dask.distributed import as_completed


Is this try/except needed? distributed is a dependency now (#466).

I'm fine with removing the optional check. Previous guidance was to make it option as it wasn't a dependency at the time.

vecchp · 2019-05-07T03:21:45Z

@stsievert all optional distributed checks in this PR should be removed now.

@TomAugspurger any guidance left to get this merged?

…as_completed

TomAugspurger

Thanks @vecchp, and apologies once again for the delay on reviewing.

This looks quite good. My only request is a small compatibility layer for build_graph.

TomAugspurger · 2019-05-07T19:44:36Z

dask_ml/model_selection/_search.py

+    return fit_params
+
+
+def build_cv_graph(


Can we provide a compatibility shim with the old name? I believe that TPOT is using build_graph https://github.com/EpistasisLab/tpot/blob/b626271e6b5896a73fb9d7d29bebc7aa9100772e/tpot/gp_deap.py#L429

I think we can just add back build_graph with a comment that it's going to be removed (not even a deprecation warning). This is a private module, but I was lazy when adding dask support to TPOT. Once this is in, I can cut a release and make a PR to tpot updating their call.

TomAugspurger · 2019-05-20T18:10:11Z

I'm looking into the compatibility for tpot now. Will push an update.

vecchp · 2019-05-20T18:11:32Z

Sounds good!

TomAugspurger · 2019-05-20T19:35:59Z

Thanks @vecchp!

vecchp · 2019-05-21T02:52:13Z

@TomAugspurger thanks so much for getting this in!

in progress updates

ce3e404

clean up a bit more

6b3688a

Paul Vecchio added 15 commits December 2, 2018 00:08

note on futures possibly returning out of order

9a8585d

ensure scores from futures line up with parameters

6e4123e

update comment about score order

a506d58

revert candidate_param change given we are not using it anymore

1fa69e3

loop over keys instead of items

578c5f8

take first value using iteration instead of index

12892fa

optional import for distributed

2c1d005

add distributed scheduler to param test

c9d3a75

put client first and close it so it doesn't leak into other tests. is…

e81f9e5

… there a better way?

remove duplicate import

000059d

remove test as it already existed

6c83b91

actually revert tests

ee34b12

ensure dask distributed exists before we check for the client

2d0627c

in progress tests for distributed as_completed cv

fa77f19

trying to figure out a way to test this

a0ad0d6

jcrist requested changes Dec 3, 2018

View reviewed changes

Paul Vecchio added 5 commits December 4, 2018 22:51

split up graphs

184e728

Merge branch 'master' into gather_results_as_completed

915cae3

Change module import and dask detection.

406f6be

Formatting to match master.

a587fa2

more formatting.

66f245e

Paul Vecchio added 4 commits December 5, 2018 00:33

fix sklearn tests and comment out as completed tests for now.

cfdc7a8

move dask graph and n_splits to the proper place.

0871f35

move dask_graph and n_splits next to results given they are all modif…

61ea9dc

…ying private attributes.

add back try/catch for distributed model

111d77a

Paul Vecchio added 7 commits February 14, 2019 23:40

need to use AsCompletedEstimator in super

8ef5338

black reformatting

869e8b9

move liter eval out of lock

9c034ab

don't need fit and transform for AsCompleted Test

e482ed9

use built in reschedule for as_completed

9008744

retry in batches

7623003

black reformatting

9da5b8f

Paul Vecchio added 2 commits May 4, 2019 09:54

remove dead code

e86bc1f

Merge branch 'master' of https://github.com/dask/dask-ml into gather_…

3db7c28

…results_as_completed

stsievert reviewed May 4, 2019

View reviewed changes

Paul Vecchio and others added 2 commits May 4, 2019 16:24

Remove optional distributed check as it is a dependency now

687a83b

Remove has_distributed check from tests

1eb113c

Merge branch 'master' of github.com:dask/dask-ml into gather_results_…

163f3bd

…as_completed

TomAugspurger reviewed May 20, 2019

View reviewed changes

TomAugspurger added 2 commits May 20, 2019 13:47

Add compat for TPOT

d5b95e4

expecte token

33f6fd2

TomAugspurger approved these changes May 20, 2019

View reviewed changes

TomAugspurger merged commit 954d41f into dask:master May 20, 2019

vecchp deleted the gather_results_as_completed branch May 21, 2019 02:52

zexuan-zhou mentioned this pull request Aug 14, 2019

update dask-ml to v1.0.0 ZEFR-INC/dask-ml#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gather CV Results as Completed #433

Gather CV Results as Completed #433

vecchp commented Dec 1, 2018 •

edited

Loading

TomAugspurger commented Dec 1, 2018

vecchp commented Dec 2, 2018 •

edited

Loading

vecchp commented Dec 5, 2018 •

edited

Loading

vecchp commented Apr 18, 2019

TomAugspurger commented Apr 18, 2019 via email •

edited

Loading

TomAugspurger commented Apr 18, 2019

vecchp commented May 4, 2019

vecchp commented May 4, 2019

stsievert May 4, 2019

vecchp May 4, 2019

vecchp commented May 7, 2019

TomAugspurger left a comment

TomAugspurger May 7, 2019

TomAugspurger May 20, 2019

TomAugspurger commented May 20, 2019

vecchp commented May 20, 2019

TomAugspurger commented May 20, 2019

vecchp commented May 21, 2019

Gather CV Results as Completed #433

Gather CV Results as Completed #433

Conversation

vecchp commented Dec 1, 2018 • edited Loading

TomAugspurger commented Dec 1, 2018

vecchp commented Dec 2, 2018 • edited Loading

vecchp commented Dec 5, 2018 • edited Loading

vecchp commented Apr 18, 2019

TomAugspurger commented Apr 18, 2019 via email • edited Loading

TomAugspurger commented Apr 18, 2019

vecchp commented May 4, 2019

vecchp commented May 4, 2019

stsievert May 4, 2019

Choose a reason for hiding this comment

vecchp May 4, 2019

Choose a reason for hiding this comment

vecchp commented May 7, 2019

TomAugspurger left a comment

Choose a reason for hiding this comment

TomAugspurger May 7, 2019

Choose a reason for hiding this comment

TomAugspurger May 20, 2019

Choose a reason for hiding this comment

TomAugspurger commented May 20, 2019

vecchp commented May 20, 2019

TomAugspurger commented May 20, 2019

vecchp commented May 21, 2019

vecchp commented Dec 1, 2018 •

edited

Loading

vecchp commented Dec 2, 2018 •

edited

Loading

vecchp commented Dec 5, 2018 •

edited

Loading

TomAugspurger commented Apr 18, 2019 via email •

edited

Loading