close
Skip to content

fix: match v* when extracting a zarr version from git tags#3994

Merged
d-v-b merged 3 commits into
zarr-developers:mainfrom
d-v-b:exclude-zarr-metadata-tags
May 21, 2026
Merged

fix: match v* when extracting a zarr version from git tags#3994
d-v-b merged 3 commits into
zarr-developers:mainfrom
d-v-b:exclude-zarr-metadata-tags

Conversation

@d-v-b
Copy link
Copy Markdown
Contributor

@d-v-b d-v-b commented May 20, 2026

This is high priority, as failing to apply this fix before our next release will ship a zarr that pretends to be much younger than it is.

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/user-guide/*.md
  • Changes documented as a new file in changes/
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@github-actions github-actions Bot added the needs release notes Automatically applied to PRs which haven't added release notes label May 20, 2026
@d-v-b d-v-b added the downstream Downstream libraries using zarr label May 20, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.39%. Comparing base (27abff2) to head (b34b540).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3994   +/-   ##
=======================================
  Coverage   93.39%   93.39%           
=======================================
  Files          88       88           
  Lines       11839    11839           
=======================================
  Hits        11057    11057           
  Misses        782      782           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@d-v-b
Copy link
Copy Markdown
Contributor Author

d-v-b commented May 20, 2026

closes #3993

@d-v-b d-v-b added run-downstream Run the tests of downstream libraries (e.g., xarray) against zarr and removed downstream Downstream libraries using zarr labels May 20, 2026
@d-v-b d-v-b requested review from maxrjones and normanrz May 21, 2026 06:23
@maxrjones
Copy link
Copy Markdown
Member

Is there a way to test this in the CI?

@maxrjones
Copy link
Copy Markdown
Member

Is there a way to test this in the CI?

Seems like we could use the test structure that numcodecs uses in https://github.com/zarr-developers/zarr-python/actions/runs/26187664671/job/77047478983?pr=3992. Could you please add a test @d-v-b?

@d-v-b
Copy link
Copy Markdown
Contributor Author

d-v-b commented May 21, 2026

The problem here is that zarr derives its version from the wrong git tag at runtime. That numcodecs test job fails because no numcodecs tests are collected, because the entire test file is skipped on zarr versions < 3.1.3: https://github.com/zarr-developers/numcodecs/blob/fa92ee7ec031479e48098f57604fed313cc72986/tests/test_zarr3.py#L10

I don't think we want to codify this behavior -- failing if zarr's runtime version is < "3.1.3" -- in a test.

@d-v-b
Copy link
Copy Markdown
Contributor Author

d-v-b commented May 21, 2026

an automated test would be valuable if we were planning on making a lot of changes to our git tag conventions. but I don't think we are, so I'm not really sure a test makes sense here, vs manually checking that the zarr version is correctly inferred on this branch.

@d-v-b
Copy link
Copy Markdown
Contributor Author

d-v-b commented May 21, 2026

uv run python -c "import zarr; print(zarr.__version__)"

main:

0.2.0

this branch:

3.2.2.dev30+gdc5e1825

Copy link
Copy Markdown
Member

@maxrjones maxrjones left a comment

Choose a reason for hiding this comment

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

I generally think we should avoid minimalism in testing and manual testing whenever possible. I think it's not good that we only caught this error because we were running numcodec's tests in our CI. That suggests to me that our version testing infrastructure is insufficient. Testing for some reasonable version structure like a regex similar v3.*.* seems like a small addition with few downsides that would prevent future issues and rarely need changing. But I'm not going to block merging on that.

@d-v-b
Copy link
Copy Markdown
Contributor Author

d-v-b commented May 21, 2026

Testing for some reasonable version structure like a regex similar v3.. seems like a small addition with few downsides that would prevent future issues and rarely need changing. But I'm not going to block merging on that.

what specific test do you propose? Testing for 3.x would break when we release 4.x we could e.g. test that the current version is greater than the previous version, but that would break if we choose a different versioning convention.

Comment thread tests/test_version_derivation.py Outdated
@@ -0,0 +1,162 @@
"""Tests for the git-tag-based version derivation configured in `pyproject.toml`.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@maxrjones have a look, this is a test that would have failed without the fix in this PR.

I'm not really sure it's worth the code, and it feels brittle because it requires parsing our pyproject.toml file. Open to ideas for simplifying this and / or finding a better way to to test the invariant (that tags starting with v, and not tags starting with zarr-metadata-v, contribute to the output of git describe, given the extra parameters to that command defined in pyproject.toml)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what about the suggestion in #3994 (comment)? It seems much less fragile and would catch bugs of this class

@maxrjones
Copy link
Copy Markdown
Member

what specific test do you propose?

"""Sanity check that ``zarr.__version__`` looks like a v3-or-newer release.
 
Background: zarr-python derives its version from ``git describe`` via
hatch-vcs. The repo also publishes a separate ``zarr-metadata`` subpackage
that uses ``zarr_metadata-v*`` tags. Without the ``--match v*`` filter in
``[tool.hatch] version.raw-options.git_describe_command``, ``git describe``
walks back to those subpackage tags and reports a version like ``0.2.0`` for
a from-source build of zarr-python itself — see
https://github.com/zarr-developers/zarr-python/pull/3994.
 
This test catches that class of regression: anything that makes zarr-python
report a version lower than the v3 release line. When 4.0 is released,
bump the floor; that's a deliberate, one-line edit at a planned boundary.
"""
 
from __future__ import annotations
 
from packaging.version import Version
 
import zarr
 
 
def test_version_is_v3_or_newer() -> None:
    # Use packaging.Version so we transparently handle hatch-vcs dev suffixes
    # like "3.2.2.dev30+gdc5e1825" that appear on any source build past the
    # latest v* tag — Version.major returns 3 for that string.
    parsed = Version(zarr.__version__)
    assert parsed.major >= 3, (
        f"zarr.__version__={zarr.__version__!r} is not on the v3 (or newer) "
        f"release line. If this fires on a from-source build, check that "
        f"[tool.hatch] version.raw-options.git_describe_command in "
        f"pyproject.toml still includes ``--match v*`` so the "
        f"``zarr_metadata-v*`` subpackage tags are excluded. See PR #3994."
    )

@d-v-b
Copy link
Copy Markdown
Contributor Author

d-v-b commented May 21, 2026

what specific test do you propose?

"""Sanity check that ``zarr.__version__`` looks like a v3-or-newer release.
 
Background: zarr-python derives its version from ``git describe`` via
hatch-vcs. The repo also publishes a separate ``zarr-metadata`` subpackage
that uses ``zarr_metadata-v*`` tags. Without the ``--match v*`` filter in
``[tool.hatch] version.raw-options.git_describe_command``, ``git describe``
walks back to those subpackage tags and reports a version like ``0.2.0`` for
a from-source build of zarr-python itself — see
https://github.com/zarr-developers/zarr-python/pull/3994.
 
This test catches that class of regression: anything that makes zarr-python
report a version lower than the v3 release line. When 4.0 is released,
bump the floor; that's a deliberate, one-line edit at a planned boundary.
"""
 
from __future__ import annotations
 
from packaging.version import Version
 
import zarr
 
 
def test_version_is_v3_or_newer() -> None:
    # Use packaging.Version so we transparently handle hatch-vcs dev suffixes
    # like "3.2.2.dev30+gdc5e1825" that appear on any source build past the
    # latest v* tag — Version.major returns 3 for that string.
    parsed = Version(zarr.__version__)
    assert parsed.major >= 3, (
        f"zarr.__version__={zarr.__version__!r} is not on the v3 (or newer) "
        f"release line. If this fires on a from-source build, check that "
        f"[tool.hatch] version.raw-options.git_describe_command in "
        f"pyproject.toml still includes ``--match v*`` so the "
        f"``zarr_metadata-v*`` subpackage tags are excluded. See PR #3994."
    )

I'm fine taking this but just not that it will fail to catch the case where the tagged version of a subpackage is greater than zarr's version, and that subpackage's git tag interferes with zarr's version inference. It will also fail if we ever change our versioning scheme.

@maxrjones
Copy link
Copy Markdown
Member

I'm fine taking this but just not that it will fail to catch the case where the tagged version of a subpackage is greater than zarr's version, and that subpackage's git tag interferes with zarr's version inference.

yeah, I think that's an ok risk.

It will also fail if we ever change our versioning scheme.

I think this is a good thing; it means our change in versioning scheme must be intentional and involve updating the test.

@d-v-b
Copy link
Copy Markdown
Contributor Author

d-v-b commented May 21, 2026

I'm fine taking this but just not that it will fail to catch the case where the tagged version of a subpackage is greater than zarr's version, and that subpackage's git tag interferes with zarr's version inference.

yeah, I think that's an ok risk.

It will also fail if we ever change our versioning scheme.

I think this is a good thing; it means our change in versioning scheme must be intentional and involve updating the test.

to be more precise, it might fail if we change our versioning scheme, but it also might not, because it doesn't test for a specific versioning scheme, just that whatever version we use sorts after "3". But I think this risk is pretty low, so I'm going to merge this pr with this test. Thanks for your help max!

@d-v-b d-v-b enabled auto-merge (squash) May 21, 2026 16:57
@d-v-b d-v-b merged commit 5ca1690 into zarr-developers:main May 21, 2026
30 checks passed
@d-v-b d-v-b deleted the exclude-zarr-metadata-tags branch May 21, 2026 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs release notes Automatically applied to PRs which haven't added release notes run-downstream Run the tests of downstream libraries (e.g., xarray) against zarr

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants