Skip to content

refactor warnings #3098

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 31 commits into from
Aug 5, 2025
Merged

refactor warnings #3098

merged 31 commits into from
Aug 5, 2025

Conversation

d-v-b
Copy link
Contributor

@d-v-b d-v-b commented May 25, 2025

closes #3096

this PR adds zarr-specific subclasses of FutureWarning and DeprecationWarning, so that we can expose routines for silencing future / deprecation warnings specifically emitted by zarr-python.

This PR is a draft until I add the silencing routines later in this PR.

@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label May 25, 2025
@dstansby
Copy link
Contributor

👍 to having our own zarr warning sub-classes, I think I'm 👎 on having some of our own warning silencing helpers, but depends on what the proposal is I guess. So would be good to have separate PRs for those.

@d-v-b
Copy link
Contributor Author

d-v-b commented Jun 30, 2025

I amended the PR description to remove the goal of providing top-level functions for users to silence zarr warnings. For now, users can use the built-in warnings library to control warnings.

@d-v-b d-v-b marked this pull request as ready for review June 30, 2025 08:48
@d-v-b d-v-b requested review from jhamman and TomAugspurger June 30, 2025 08:48
@d-v-b d-v-b added this to the 3.0.9 milestone Jun 30, 2025
@d-v-b d-v-b removed this from the 3.0.9 milestone Jun 30, 2025
Copy link

codecov bot commented Jul 11, 2025

Codecov Report

❌ Patch coverage is 70.90909% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.54%. Comparing base (e985f1d) to head (61ff062).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/zarr/api/asynchronous.py 33.33% 12 Missing ⚠️
src/zarr/_compat.py 0.00% 1 Missing ⚠️
src/zarr/convenience.py 0.00% 1 Missing ⚠️
src/zarr/creation.py 0.00% 1 Missing ⚠️
src/zarr/testing/__init__.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3098      +/-   ##
==========================================
+ Coverage   94.40%   94.54%   +0.13%     
==========================================
  Files          78       78              
  Lines        9401     9419      +18     
==========================================
+ Hits         8875     8905      +30     
+ Misses        526      514      -12     
Files with missing lines Coverage Δ
src/zarr/api/synchronous.py 92.95% <100.00%> (+0.10%) ⬆️
src/zarr/codecs/transpose.py 89.47% <ø> (ø)
src/zarr/core/array.py 97.10% <100.00%> (-0.33%) ⬇️
src/zarr/core/buffer/gpu.py 90.00% <100.00%> (+0.12%) ⬆️
src/zarr/core/chunk_grids.py 91.58% <100.00%> (+0.07%) ⬆️
src/zarr/core/codec_pipeline.py 93.13% <100.00%> (-0.96%) ⬇️
src/zarr/core/common.py 93.38% <100.00%> (+0.05%) ⬆️
src/zarr/core/dtype/common.py 85.54% <100.00%> (ø)
src/zarr/core/group.py 95.03% <100.00%> (+0.22%) ⬆️
src/zarr/core/metadata/v2.py 91.27% <100.00%> (+0.05%) ⬆️
... and 9 more

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dstansby dstansby added this to the 3.2.0 milestone Jul 31, 2025
@github-actions github-actions bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Jul 31, 2025
@d-v-b d-v-b requested a review from a team July 31, 2025 21:59
@d-v-b
Copy link
Contributor Author

d-v-b commented Jul 31, 2025

this will allow users who are tired of seeing zarr-specific warnings to suppress them.

@dstansby
Copy link
Contributor

dstansby commented Aug 1, 2025

I milestoned as 3.2.0, but I guess this isn't breaking (because the new warnings subclass the old warnings), so could go in 3.1.2 instead?

Copy link
Contributor

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

Code changes look good overall, but:

  • Tests need fixing
  • I think there's more warnings in the list of our warning filters that should be converted to the new zarr-specific warnings
  • The new warning classes shold be included in the API docs

@d-v-b
Copy link
Contributor Author

d-v-b commented Aug 1, 2025

I removed almost all the ignored warnings from pyproject.toml, at the cost of inserting a bunch of explicit with pytest.warns() statements throughout our tests. This was useful because it revealed how many warnings zarr emits during normal usage. For consolidated metadata in particular I think we need to rethink emitting a warning every time its used. I don't think this feature is typically used by individual users but rather libraries like xarray, in which case the best way to convey the off-spec-ness of consolidated metadata is via documentation.

@d-v-b
Copy link
Contributor Author

d-v-b commented Aug 1, 2025

we should also consider changing our ZipStore implementation to hide the UserWarning emitted whenever a key is modified.

@TomAugspurger
Copy link
Contributor

removed almost all the ignored warnings from pyproject.toml, at the cost of inserting a bunch of explicit with pytest.warns() statements throughout our tests

Thanks for doing this. I was going to suggest that, but didn't want to assign work to you that's only tangentially related to the PR. I think that being explicit in the tests about where warnings are raised is a good practice.

@d-v-b
Copy link
Contributor Author

d-v-b commented Aug 1, 2025

consistent with my "test code models user code" theory, excluding every warning individually from the tests has given me a newfound appreciation for all the warnings zarr-python emits on a regular basis

@d-v-b
Copy link
Contributor Author

d-v-b commented Aug 1, 2025

I don't think the coverage report is very useful here. If we discount it, I think this is ready to go

@d-v-b d-v-b requested a review from dstansby August 1, 2025 14:01
@d-v-b d-v-b requested review from a team and dstansby August 4, 2025 14:02
@dstansby dstansby modified the milestones: 3.2.0, 3.1.2 Aug 5, 2025
Copy link
Contributor

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

One final comment on the release note, otherwise 👍 Once that's fixed, feel free to self-merge

@d-v-b d-v-b merged commit 71cc0c2 into zarr-developers:main Aug 5, 2025
30 of 31 checks passed
@d-v-b d-v-b deleted the refactor-warnings branch August 5, 2025 17:06
meeseeksmachine pushed a commit to meeseeksmachine/zarr-python that referenced this pull request Aug 5, 2025
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.

improve control over warnings
3 participants