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

Log popen stdout/err when subprocess times out #6567

Merged
merged 5 commits into from
Jun 15, 2022

Conversation

gjoseph92
Copy link
Collaborator

Progress towards #6395. This should give us visibility into what's happening when the subprocess times out.

cc @crusaderky @mrocklin

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

@github-actions
Copy link
Contributor

github-actions bot commented Jun 12, 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 31m 51s ⏱️ + 2m 23s
  2 867 tests +  4    2 787 ✔️ +  8    80 💤  - 1  0  - 1 
21 238 runs  +26  20 300 ✔️ +32  938 💤  - 3  0  - 1 

Results for commit 9309c4f. ± Comparison against base commit 058e629.

♻️ This comment has been updated with latest results.

@fjetter
Copy link
Member

fjetter commented Jun 13, 2022

Doesn't seem to work on windows

@gjoseph92 gjoseph92 self-assigned this Jun 13, 2022
@gjoseph92 gjoseph92 added the tests Unit tests and/or continuous integration label Jun 13, 2022
@gjoseph92
Copy link
Collaborator Author

Yeah. I think that makes sense because Windows doesn't have SIGINT vs SIGKILL. terminate is the same as kill. So the process probably gets killed right away instead of catching KeyboardInterrupt. I'll look into what to do instead for windows.

pytest will suppress it if the test passes
Printing the logs will likely help diagnose why this test is hanging.
This is a shot in the dark
* different exit code on windows (not -9)
* `/bin/echo` obvs doesn't exist
@gjoseph92
Copy link
Collaborator Author

Wooo it's green! Ready to merge @fjetter @crusaderky.

@fjetter fjetter merged commit bc90846 into dask:main Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Unit tests and/or continuous integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants