-
Notifications
You must be signed in to change notification settings - Fork 89
Refactor dask tests #2377
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
Refactor dask tests #2377
Conversation
1575ff9
to
677f0c4
Compare
Codecov Report
@@ Coverage Diff @@
## main #2377 +/- ##
=======================================
+ Coverage 99.9% 99.9% +0.1%
=======================================
Files 281 281
Lines 24849 24858 +9
=======================================
+ Hits 24817 24826 +9
Misses 32 32
Continue to review full report at Codecov.
|
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, I like this refactor a lot and it aligns more with how our other unit tests work too. And of course, the almost-consistently green CI makes me happy 😁
@@ -7,6 +7,7 @@ Release Notes | |||
* Documentation Changes | |||
* Testing Changes | |||
* Add ``pytest-timeout``. All tests that run longer than 6 minutes will fail. :pr:`2374` | |||
* Refactored dask tests :pr:`2377` |
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.
omega nit-pick but maybe include why so that when we try to trace this back in the future we know? :P
We should probably standby for a little bit to see whether turning off the broadcast option helps, like in PR #2376. I've been tracking some of the speculation and resolution on the source issue, #2354 . The dask issue that led me to try turning off broadcast was here. I'd be hesitant to move away from the single client for testing only because it should work that way, haha. |
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.
Nice changes!
Pull Request Description
Refactor dask tests to not reuse a client across tests. Also use the "standard" pytest workflow of writing functions instead of classes. This was inspired by how lightgbm tests dask.
https://github.com/microsoft/LightGBM/pull/4159/files
I pushed a commit every three hours over the weekend and the tests always passed. Note the following:
A lot of the red x's are because the
codecov/patch
check did not pass (I believe this is a "bug" stemming from the fact there was no diff over repeated commits. The issue fixed itself though).This does not fix this issue Flaky Dask test #2341, and it's why the windows unit tests failed for commit
5d57ebb
. However,dask_sends_woodwork_schema
did not fail.I still do not have a 100% confident explanation of what was causing the original error. But I don't think this was a gh problem after all since I was finally able to repro the timeout locally.
My hand-wavy explanation is that the dask scheduler somehow is losing track of the data we've scattered. Since the client is the scheduler and we're reusing the same client across the test, it made me think that some global state was being corrupted. I saw that lightgbm and xgboost do not reuse clients across tests and it gave me the idea to try it here.
I filed an issue with
dask/distributed
as well dask/distributed#4907After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of
docs/source/release_notes.rst
to include this pull request by adding :pr:123
.