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

TST: Use safer context for ProcessPoolExecutor #8715

Merged
merged 1 commit into from
Jun 25, 2024
Merged

Conversation

QuLogic
Copy link
Contributor

@QuLogic QuLogic commented Jun 21, 2024

When running test_process_executor_kills_process just by itself, I get a warning that threads have been used and that the fork multiprocessing context is unsafe. If I run the full test suite, then the same test hangs.

I don't know where the threads are coming from (as I didn't use xdist), but explicitly use the safer spawn multiprocess context to avoid these issues.

  • Tests added / passed
  • Passes pre-commit run --all-files

@QuLogic QuLogic requested a review from fjetter as a code owner June 21, 2024 20:54
Copy link
Contributor

github-actions bot commented Jun 21, 2024

Unit Test Results

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

    29 files  ±0      29 suites  ±0   11h 27m 43s ⏱️ - 7m 33s
 4 064 tests ±0   3 963 ✅ +3     97 💤 ±0  4 ❌  - 3 
55 981 runs  ±0  53 812 ✅ +4  2 164 💤 ±0  5 ❌  - 4 

For more details on these failures, see this check.

Results for commit efb95e4. ± Comparison against base commit 5b2e2b4.

♻️ This comment has been updated with latest results.

@@ -2199,7 +2200,7 @@ async def test_bad_executor_annotation(c, s, a, b):

@gen_cluster(client=True)
async def test_process_executor(c, s, a, b):
with ProcessPoolExecutor() as e:
with ProcessPoolExecutor(mp_context=multiprocessing.get_context("spawn")) as e:
Copy link
Member

Choose a reason for hiding this comment

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

Might be best to use distributed.utils.get_mp_context which picks the context that is being configured by default. It also sets some preload things for a forkserver which isn't truly relevant. However, this would make the tests sensitive to what we're setting as defaults which sounds like a good thing to have

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, can do that.

When running `test_process_executor_kills_process` just by itself, I get
a warning that threads have been used and that the `fork`
multiprocessing context is unsafe. If I run the full test suite, then
the same test hangs.

I don't know where the threads are coming from (as I didn't use xdist),
but explicitly use distributed's default context (which is the safer
`spawn` multiprocess context) to avoid these issues.
@fjetter fjetter merged commit 97dbdaa into dask:main Jun 25, 2024
30 of 36 checks passed
@QuLogic QuLogic deleted the test-spawn branch June 25, 2024 09:20
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.

2 participants