Skip to content
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

Rewrite test_reconnect to use subproc to kill scheduler reliably #6967

Merged
merged 2 commits into from
Aug 30, 2022

Conversation

fjetter
Copy link
Member

@fjetter fjetter commented Aug 29, 2022

I noticed that even on a successful run of this test, we get a lot of "AsyncioGroup already closed" etc. errors since we clearly cripple the running scheduler by closing all the internals manually. Particularly the client and worker disconnects can trigger us trying to schedule messages to deal with the loss.

Looking at the test, we can see a lot of CancelledErrors with long tracebacks. I'm wondering if we're simply hitting #6211 and #6847 is not responsible after all but merely changed timing such that we hit this condition more reliably.

Let's see what CI thinks about this theory.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 29, 2022

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       15 files  ±0         15 suites  ±0   6h 35m 55s ⏱️ - 7m 18s
  3 052 tests +1    2 965 ✔️  - 3    84 💤 +1  3 +3 
22 577 runs  +8  21 601 ✔️ +8  973 💤  - 3  3 +3 

For more details on these failures, see this check.

Results for commit e5a6c2d. ± Comparison against base commit c083790.

♻️ This comment has been updated with latest results.

@fjetter fjetter changed the title WIP Rewrite test_reconnect to use subproc to kill scheduler reliably Rewrite test_reconnect to use subproc to kill scheduler reliably Aug 29, 2022
@fjetter fjetter marked this pull request as ready for review August 29, 2022 10:17
@fjetter
Copy link
Member Author

fjetter commented Aug 29, 2022

🟢 🎉

Copy link
Collaborator

@gjoseph92 gjoseph92 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this. Couple style nits, take them or leave them

distributed/tests/test_client.py Outdated Show resolved Hide resolved
distributed/tests/test_client.py Show resolved Hide resolved
Co-authored-by: Gabe Joseph <gjoseph92@gmail.com>
Copy link
Member

@hendrikmakait hendrikmakait left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for taking care of fixing this!

@fjetter fjetter merged commit 6a1b089 into dask:main Aug 30, 2022
@fjetter fjetter deleted the rewrite_test_reconnect branch August 30, 2022 09:34
gjoseph92 added a commit to gjoseph92/distributed that referenced this pull request Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants