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

test(ci): test on windows and osx #2431

Merged
merged 14 commits into from
Oct 24, 2024

Conversation

jhamman
Copy link
Member

@jhamman jhamman commented Oct 22, 2024

This PR adds CI matrix entries for OSX and Windows.

Closes #1648

Notes:

There are some interesting failures here.

- [ ] path handling in local store

  • async timeout error in osx

@jhamman jhamman marked this pull request as ready for review October 22, 2024 18:03
@jhamman
Copy link
Member Author

jhamman commented Oct 23, 2024

@zarr-developers/python-core-devs - do we have any Window's aficionados capable developers here? We need to do a bit of work to tweak path handling in the LocalStore and it would be approx. 1000% easier to iterate locally than via GitHub actions! The good news is that we're super close to having all the tests passing :)

@martindurant
Copy link
Member

tweak path handling in the LocalStore

fsspec has a decent amount of work for this which you can reuse. Actually, could RemoteStore write to local too to avoid the duplication, or would that be unnecessarily complicated (given the async/sync difference in particular).

@jhamman
Copy link
Member Author

jhamman commented Oct 23, 2024

fsspec has a decent amount of work for this which you can reuse.

Can you provide some pointers?

Actually, could RemoteStore write to local too to avoid the duplication, or would that be unnecessarily complicated (given the async/sync difference in particular).

I don't think this will help us because the RemoteStore requires an Async Filesystem which the fsspec local filesystem is not.

@martindurant
Copy link
Member

fsspec calls https://github.com/fsspec/filesystem_spec/blob/master/fsspec/implementations/local.py#L284 as early as possible in localFS, since posix paths always work on windows in python (but not the other way around).

@jhamman jhamman mentioned this pull request Oct 24, 2024
src/zarr/storage/local.py Outdated Show resolved Hide resolved
src/zarr/storage/local.py Outdated Show resolved Hide resolved
Comment on lines +36 to +43
# - python-version: '3.11'
# numpy-version: '1.25'
# dependency-set: 'optional'
# os: 'windows-latest'
# - python-version: '3.13'
# numpy-version: '2.1'
# dependency-set: 'optional'
# os: 'windows-latest'
Copy link
Member Author

Choose a reason for hiding this comment

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

I've opened a dedicated ticket for this: #2438

@jhamman jhamman requested a review from dcherian October 24, 2024 23:00
@jhamman
Copy link
Member Author

jhamman commented Oct 24, 2024

pre-commit.ci autofix

@jhamman jhamman merged commit 4a3bbf1 into zarr-developers:main Oct 24, 2024
24 checks passed
@jhamman jhamman deleted the ci/expand-ci-matrix branch October 24, 2024 23:27
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.

[v3] Test environments and CI
3 participants