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

Pin numcodecs to version that support python 3.10 #549

Open
wants to merge 7 commits into
base: branch-24.12
Choose a base branch
from

Conversation

robertmaynard
Copy link
Contributor

No description provided.

@robertmaynard robertmaynard added bug Something isn't working non-breaking Introduces a non-breaking change labels Nov 14, 2024
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Can we allow newer numcodecs versions on newer Python versions? Why is the upper bound necessary? Are there significant incompatibilities between 0.13 and 0.14?

Also needs an update here:

- numcodecs !=0.12.0

dependencies.yaml Outdated Show resolved Hide resolved
Co-authored-by: Bradley Dice <[email protected]>
@robertmaynard
Copy link
Contributor Author

Can we allow newer numcodecs versions on newer Python versions? Why is the upper bound necessary? Are there significant incompatibilities between 0.13 and 0.14?

The 0.14 pypi package doesn't offer python 3.10 support. Outside of that I have no idea what kind of compatibility there is between releases.

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Rob! 🙏

Added suggestions below for the pinnings. Think we need them a bit tighter

conda/environments/all_cuda-118_arch-aarch64.yaml Outdated Show resolved Hide resolved
conda/environments/all_cuda-118_arch-x86_64.yaml Outdated Show resolved Hide resolved
conda/environments/all_cuda-125_arch-aarch64.yaml Outdated Show resolved Hide resolved
conda/environments/all_cuda-125_arch-x86_64.yaml Outdated Show resolved Hide resolved
conda/recipes/kvikio/meta.yaml Outdated Show resolved Hide resolved
python/kvikio/pyproject.toml Outdated Show resolved Hide resolved
@jakirkham
Copy link
Member

Can we allow newer numcodecs versions on newer Python versions? Why is the upper bound necessary? Are there significant incompatibilities between 0.13 and 0.14?

This is a good question. Perhaps first it is helpful to provide some context around the existing pin?

We added this pin due to a bug in Numcodecs' entrypoint handling logic that occurred in 0.12.0 and was fixed in 0.12.1 with Vyas' PR ( zarr-developers/numcodecs#475 ). So we just excluded the problematic version here. AIUI Rob is keeping with that and simply bumping the minimum version past the problematic version giving us 0.13

As to newer versions of Numcodecs, periodically it does add new optional codecs. Sometimes there are small fixes or improvements to existing ones. The API has been pretty stable for a while

Looking at the diff between 0.12.1 and 0.14, it appears Python 3.10 was dropped (as discussed above), some dependencies were bumped or new versions supported (like NumPy 2), Zarr v3 (unreleased) support was added, a new cyclic redundancy check (CRC) with optional dependency was added, existing CRCs got a new optional flag, a performance fix to the Delta codec was made, a multiprocessing bug fix, a breaking change with Zstd attributes (don't think we use these), etc. In terms of API breaks am not seeing much to worry about here. Think that will likely be true in the future (though we should keep an eye on how Zarr v3 support in Numcodecs evolves)


Think the real question is, are we ok with having some drift between what our oldest supported Python version (currently 3.10) and what our newest supported Python version (currently 3.12) can be installed with?

And if so, are there any changes we may need to make elsewhere to adapt to this reality? If not, should we start constraining all our dependencies based on the oldest Python version we support (and when do we refresh them)?

Admittedly we might want to have this discussion in somewhere more general than this PR for greater visibility and easier referencing. Though wanted to follow up here since this is where the questions came up

@bdice
Copy link
Contributor

bdice commented Nov 15, 2024

Think the real question is, are we ok with having some drift between what our oldest supported Python version (currently 3.10) and what our newest supported Python version (currently 3.12) can be installed with?

Yes, we are usually happy with allowing drift here. If we pin all our dependencies to only those version ranges that work with all our supported Python versions, we would be too tightly pinned for broad compatibility with the Python ecosystem. If the numcodecs APIs we use are compatible with 0.13 and 0.14, we should not exclude 0.14.

@robertmaynard
Copy link
Contributor Author

I have expanded the comment in the dependencies.yaml with more context on the pinning.

@robertmaynard robertmaynard marked this pull request as ready for review November 15, 2024 16:42
@robertmaynard robertmaynard requested a review from a team as a code owner November 15, 2024 16:42
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

I have expanded the comment in the dependencies.yaml with more context on the pinning.

@robertmaynard Did you push? I don't see the context / comments. Blocking merge until we can figure out why this needs the upper bound.

@@ -356,7 +356,7 @@ dependencies:
- numpy>=1.23,<3.0a0
- zarr
# See https://github.com/zarr-developers/numcodecs/pull/475
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the PR linked above still relevant? I think it may be safe to remove this comment now.

conda/recipes/kvikio/meta.yaml Show resolved Hide resolved
@robertmaynard
Copy link
Contributor Author

I have expanded the comment in the dependencies.yaml with more context on the pinning.

@robertmaynard Did you push? I don't see the context / comments. Blocking merge until we can figure out why this needs the upper bound.

Somehow didn't push. Updated now

Comment on lines 358 to 361
# Pinned to the 0.13 for the following reasons:
# https://github.com/zarr-developers/numcodecs/pull/475
# 0.14.0 doesn't offer python 3.10 support
- numcodecs>=0.13.0,<0.14.0a0
Copy link
Contributor

@bdice bdice Nov 20, 2024

Choose a reason for hiding this comment

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

Not every version in this range needs to support every Python version that we build for. We only need one compatible version of numcodecs in this range for each Python version we support. It is fine to let Python 3.11 and 3.12 float to numcodecs 0.14 (or newer) even if Python 3.10 can only use 0.13.

Can we relax this?

Suggested change
# Pinned to the 0.13 for the following reasons:
# https://github.com/zarr-developers/numcodecs/pull/475
# 0.14.0 doesn't offer python 3.10 support
- numcodecs>=0.13.0,<0.14.0a0
- numcodecs>=0.13.0

@robertmaynard
Copy link
Contributor Author

Can we allow newer numcodecs versions on newer Python versions? Why is the upper bound necessary? Are there significant incompatibilities between 0.13 and 0.14?

Also needs an update here:

- numcodecs !=0.12.0

Updated to allow all version >= 0.13.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants