Skip to content

MAINT: use async/await in model selection#675

Merged
TomAugspurger merged 45 commits intodask:masterfrom
stsievert:ms-async
May 28, 2020
Merged

MAINT: use async/await in model selection#675
TomAugspurger merged 45 commits intodask:masterfrom
stsievert:ms-async

Conversation

@stsievert
Copy link
Copy Markdown
Member

@stsievert stsievert commented May 27, 2020

What does this PR implement?
It uses async/await syntax instead of Tornado.

Reference issues/PRs
Hopefully this resolves #673.

stsievert and others added 30 commits June 21, 2019 17:35
@stsievert
ENH: prioritize model selection fitting by score
2bb66de
@stsievert
lint
5c21e90
@stsievert
MAINT: add random noise to priorities
7236c8c
@stsievert
MAINT: better explanation, debug
94324d4
@stsievert
always define variables
f6b0def
@stsievert
properly reflect comments
07d4a58
@stsievert
Priority based on model rank (not score)
9f92511
@stsievert
Better low priority
150c186
@stsievert
Merge branch 'master' into ms-priority
4443f5a
@stsievert
Merge branch 'master' into ms-priority
3f6cbea
@stsievert
Trigger CI
85fb268
@stsievert
Trigger CI (timeouts?)
2fc1737
@stsievert
trigger CI
745f964
@stsievert
Merge branch 'master' into ms-priority
50c3c32
@stsievert
async test
af78b6d
@stsievert
lint
133bfda
@stsievert
Merge branch 'master' into ms-priority
dc12c8f
@stsievert
Remove warnings (tornado -> async)
d8269e5
@stsievert
stdout test failing but working practically?
b1c38f2
@stsievert
More async?
7d3d531
@stsievert
MAINT: make logging work with async/await
eaab0bd 
(and make sure loggers only log to unique streams)
@stsievert
black
0ce27ca
@stsievert
isort
f9bc9a5
@stsievert
black
aa43a9f
@stsievert
lint
db02126
@stsievert
bump; does test_basic fail again?
44e9563
@stsievert
remove tornado for incremental tests
7d14abf
@stsievert
typo
0f1aee8
@stsievert
remove unused code, make test run
b7477c5
@stsievert
(sigh) staged not commited
a5327ac
@stsievert stsievert marked this pull request as draft May 27, 2020 13:40
@stsievert stsievert mentioned this pull request May 27, 2020
Open
@stsievert stsievert marked this pull request as ready for review May 27, 2020 13:50
@mrocklin
Copy link
Copy Markdown
Member

mrocklin commented May 27, 2020

Regardless of if this fixes the underlying issue, hooray for modern python!

@stsievert
Copy link
Copy Markdown
Member Author

stsievert commented May 27, 2020

I think this PR is ready for merge or review. The CI has been green for a while, and I think it's stable; I've made some small tweaks to increase stability to test_incremental::test_basic:

Details

test_incremental::test_basic passes all the time locally; however, on the CI it failed in c1a3bd2. Since then, I have done the following:

  1. Changed the model from SGDClassifier to ConstantFunction. Now, no computation is required for fitting.
  2. Increased the timeout from 500 to 1000 seconds. I think this is fair; I've included timeout=None and timeout=5000 in other model selection tests.

When I time this test locally, 83% ran in less than 1 second and 89% ran in less than 3 seconds during 101 tests. 11 of these tests that took longer took between 12 and 17 seconds.

Regardless of if this fixes the underlying issue, hooray for modern python!

Now, Dask-ML doesn't import Tornado!

$ grep -r "tornado" dask_ml tests
$

TomAugspurger
TomAugspurger reviewed May 28, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

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

Thanks.

Seems like there was still a failure on windows unfortunately. Still progress though, just a few small changes.

@TomAugspurger TomAugspurger merged commit a770dbd into dask:master May 28, 2020
@TomAugspurger
Copy link
Copy Markdown
Member

TomAugspurger commented May 28, 2020

Thanks!

@stsievert stsievert deleted the ms-async branch May 28, 2020 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TomAugspurger TomAugspurger TomAugspurger left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Flaky Incremental Model Selection tests

3 participants

@stsievert @mrocklin @TomAugspurger