8000 Fix `test_local_directory` and `test_local_directory_make_new_directory` on Windows by hendrikmakait · Pull Request #6954 · dask/distributed · GitHub
[go: up one dir, main page]

Skip to content

Fix test_local_directory and test_local_directory_make_new_directory on Windows #6954

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

Merged
merged 1 commit into from
Aug 25, 2022

Conversation

hendrikmakait
Copy link
Member
@hendrikmakait hendrikmakait commented Aug 25, 2022

Closes #6934
Closes #6935

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

@hendrikmakait hendrikmakait changed the title Fix tests on Windows Fix test_local_directory and test_local_directory_make_new_directory on Windows Aug 25, 2022
@hendrikmakait
Copy link
Member Author

CI flake: #6896

@github-actions
Copy link
Contributor

Unit Test Results

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

       15 files  +       1         15 suites  +1   6h 40m 22s ⏱️ + 1h 1m 55s
  3 044 tests ±       0    2 960 ✔️ +     15    83 💤  - 12  1  - 3 
22 514 runs  +1 819  21 540 ✔️ +1 811  973 💤 +11  1  - 3 

For more details on these failures, see this check.

Results for commit e259c17. ± Comparison against base commit b8d1c7b.

Copy link
Member
@fjetter fjetter left a comment

Choose a reason for hiding this comment

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

Any idea what caused this to fail in the first place?

@fjetter fjetter merged commit 4f7c802 into dask:main Aug 25, 2022
@hendrikmakait
Copy link
Member Author
hendrikmakait commented Aug 25, 2022

Any idea what caused this to fail in the first place?

dask/dask#9429 contains some context. It looks like this might be caused by CI upgrading to Python 3.10.6 which contains this PR: python/cpython#32010. In

self._workspace = WorkSpace(os.path.abspath(local_directory))
the file now gets stripped of the trailing period.

@fjetter
Copy link
Member
fjetter commented Aug 25, 2022

good catch!

@graingert
Copy link
Member

Any idea what caused this to fail in the first place?

dask/dask#9429 contains some context. It looks like this might be caused by CI upgrading to Python 3.10.6 which contains this PR: python/cpython#32010. In

self._workspace = WorkSpace(os.path.abspath(local_directory))

the file now gets stripped of the trailing period.

Did CPython intend this as a breaking change?

assert w.local_directory.startswith(fn)
assert "dask-worker-space" in w.local_directory
async def test_local_directory(s, tmp_path):
with dask.config.set(temporary_directory=str(tmp_path)):
Copy link
Member

Choose a reason for hiding this comment

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

Nit?: I prefer to use os.fsdecode instead of str for pathlib.Path objects

@hendrikmakait
Copy link
Member Author

Did CPython intend this as a breaking change?

Good question, nonetheless, Microsoft actively discourages periods at the end of file names:

Do not end a file or directory name with a space or a period. Although the underlying file system may support such names, the Windows shell and user interface does not. However, it is acceptable to specify a period as the first character of a name. For example, ".temp".

https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file

async def test_local_directory(s, tmp_path):
with dask.config.set(temporary_directory=str(tmp_path)):
w = await Worker(s.address)
assert w.local_directory.startswith(str(tmp_path))
Copy link
Member

Choose a reason for hiding this comment

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

Fun fact: This can be done a little bit nicer by pathlib.Path(w.local_directory) and then using the pathlib https://docs.python.org/3/library/pathlib.html#pathlib.PurePath.is_relative_to if someone can sort out a release of the pathlib2 backport

gjoseph92 pushed 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.

flaky test_local_directory_make_new_directory flaky test_local_directory
3 participants
0