Skip to content

Conversation

@jameslamb
Copy link
Member

Closes #5069

Small updates to utils_test.popen():

  • uses sysconfig.get_path("scripts") to handle differences across operating systems
  • allows avoiding path modifications by passing an absolute path (e.g. popen(["/usr/local/bin/dask"]))

Notes for Reviewers

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

How I tested this

Locally, on my arm64 Mac

conda env create \
  --yes \
  --name distributed-dev \
  --file ./continuous_integration/environment-3.13.yaml

source activate distributed-dev
pip install --no-deps .
python -m pytest distributed \
  -m "not avoid_ci" \
  --leaks=fds

# === 10 failed, 3699 passed, 299 skipped, 7 deselected, 26 xfailed, 11 xpassed in 932.88s (0:15:32)

@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2026

Unit Test Results

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

    27 files  ± 0      27 suites  ±0   9h 51m 46s ⏱️ + 10m 0s
 4 113 tests + 1   4 008 ✅ + 2    104 💤 ±0  1 ❌  - 1 
51 529 runs  +13  49 343 ✅ +13  2 185 💤 +1  1 ❌  - 1 

For more details on these failures, see this check.

Results for commit 67837d6. ± Comparison against base commit 5da04d0.

♻️ This comment has been updated with latest results.

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

Thanks James! Overall I'm +1 on this change.

There are some failures on Windows that seem related though

FAILED distributed/cli/tests/test_dask_worker.py::test_dashboard_non_standard_ports - FileNotFoundError: Could not find 'C:\Users\runneradmin\miniconda3\envs\dask-distributed\Scripts\dask'. To avoid this warning, provide an absolute path to an existing installation to popen().
FAILED distributed/cli/tests/test_dask_worker.py::test_single_executable_works - FileNotFoundError: Could not find 'C:\Users\runneradmin\miniconda3\envs\dask-distributed\Scripts\dask-worker'. To avoid this warning, provide an absolute path to an existing installation to popen().
FAILED distributed/tests/test_utils_test.py::test_popen_file_not_found - Failed: Invalid regex pattern provided to 'match': incomplete escape \U at position 18

@jameslamb
Copy link
Member Author

They do seem related, thanks for pulling those out. I was able to reproduce both of these on a Windows system. I've pushed fixes to resolve them: 67837d6

The issues:

  • passing an absolute filepath as part of a regex in pytest.raises() without escaping it was a bad idea. I've just simplified the string that's checked to one that doesn't require escaping.
  • on Windows, subprocess.Popen(["dask-worker"]) will work despite the installed file being named "dask-worker.exe". os.path.isfile() is (understandably!) not as forgiving. I've added Windows-specific handling for that case.

If you see this and think the extra handling just to raise a more informative error message is not worth it, let me know and I'd be happy to remove that in favor of just passing things through to subprocess.Popen() (as is currently done in this function) and letting Popen() raise whatever errors it wants on a filepath that doesn't exist.

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.

distributed.utils_test.popen does not use binaries from PATH

2 participants