-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
[dask] fix teardown issues in Dask tests (fixes #3829) #3869
Conversation
Based on #3829 (comment), I believe this is ready to review. |
@@ -172,7 +176,7 @@ def test_classifier(output, centers, client, listen_port): | |||
assert_eq(p1_local, p2) | |||
assert_eq(y, p1_local) | |||
|
|||
client.close() | |||
client.close(timeout=CLIENT_CLOSE_TIMEOUT) |
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.
Maybe we can wrap client
as a resource-like fixture?
@pytest.fixture()
def resource():
print("setup")
yield "resource"
print("teardown")
https://stackoverflow.com/a/39401087
It will reduce code duplication and help to avoid cases where we forget to add this code line.
#3829 (comment)
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.
The client we import from distributed
is already itself a pytest
fixture: https://github.com/dask/distributed/blob/b5c36b587f0e3295fe59db330603d5c78b39331f/distributed/utils_test.py#L543-L547
I'm really nervous about adding a second layer of @pytest.fixture()
on top of this, when this PR's goal is specifically to fix an error caused by instability in teardowns. I'm worried that will expose us to new types of problems related to having fixtures-of-fixtures.
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 a lot for all your hard work!
Ok thanks for the review and conversation in #3829 ! I'm going to merge this, let's see if it helps. |
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
See #3829 (comment) for an explanation of this pull request.
The tests results below should builds I used to try to establish how often this teardown timeout issue happens. See #3829 (comment) for a summary.
This PR proposes just increasing the timeout on the
client.close()
calls used in Dask tests, to 60 seconds. It also addsclient.close()
calls to two tests that were missing them.Test runs with no changes to anything in
python-package/
ortests/
sdist
(5 Linux, 5 Linux_latest per build, Python=3.8)build 1: ✔️ 10, ❌ 0
build 2: ✔️ 10, ❌ 0
build 3: ✔️ 10, ❌ 0
build 4: ✔️ 10, ❌ 0
build 5: ✔️ 10, ❌ 0
bdist
(5 Linux, 5 Linux_latest per build, Python=3.8)build 1: ✔️ 10, ❌ 0
build 2: ✔️ 9, ❌ 1
Linux bdist timeout failure (1)
build 3: ✔️ 10, ❌ 0
build 4: ✔️ 10, ❌ 0
build 5: ✔️ 10, ❌ 0
regular
(5 Linux, 5 Linux_latest per build, Python=3.8)build 1: ✔️ 10, ❌ 0
build 2: ✔️ 8, ❌ 2
Linux regular timeout failure (2)
build 3: ✔️ 10, ❌ 0
build 4: ✔️ 10, ❌ 0
build 5: ✔️ 10, ❌ 0
bdist
(5 Linux, 5 Linux_latest per build, Python=3.7)build 1: ✔️ 10, ❌ 0
build 2: ✔️ 10, ❌ 0
build 3: ✔️ 10, ❌ 0
build 4: ✔️ 9, ❌ 1
Linux bdist timeout failure (1)
build 5: ✔️ 10, ❌ 0
regular
(5 Linux, 5 Linux_latest per build, Python=3.7)build 1: ✔️ 10, ❌ 0
build 2: ✔️ 10, ❌ 0
build 3: ✔️ 10, ❌ 0
build 4: ✔️ 10, ❌ 0
build 5: ✔️ 10, ❌ 0