-
Notifications
You must be signed in to change notification settings - Fork 50
Fix align_chunks xarray backcompat + add min version tests
#1299
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
Conversation
nice! it caught it |
| ) | ||
|
|
||
|
|
||
| @pytest.mark.skipif( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can't be right; it's been running so far!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was a bit confusing. but i saw very consistent failures on the min version tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree that this seems wrong though
|
|
||
| pytestmark = pytest.mark.skipif( | ||
| Version(zarr.__version__) < Version("3.1.0"), | ||
| reason="BytesCodec Issuess with less than 3.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we just need to skip when dtype.kind == "S"? If so we can use hypothesis.strategies.assume in the body of the test. It'd be nice to be much narrower here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will look into it
.github/workflows/python-check.yaml
Outdated
| uv pip install pytest-mypy-plugins | ||
| # Install specific xarray version based on matrix | ||
| if [ "${{ matrix.xarray-version.version }}" = "latest" ]; then | ||
| echo "Using xarray from git checkout (latest)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's have this be the latest released versions. Otherwise our PRs will start failing when there's some Xarray/Zarr incompatibility on their main branches. we already run nightly upstream tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call. Although the nightly tests don't actually test the backends :(
Also I'm somewhat shocked that this never failed in the past. it's always been checking out the latest git version, not the most recent release.
I'll look into grabbing the xarray verssion and using that to checkout, but it may get a bit complicated.

Fixes: #1298
Initially opening without the fix to check that the new min version tests introduced here would have caught this.