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

(fix): upper bound zarr at runtime (and in docs + benchmark) #1819

Merged
merged 9 commits into from
Jan 10, 2025

Conversation

ilan-gold
Copy link
Contributor

@ilan-gold ilan-gold commented Jan 9, 2025

I would make a release just for this.

  • Closes #
  • Tests added
  • Release note added (or unnecessary)

@ilan-gold ilan-gold added this to the 0.11.3 milestone Jan 9, 2025
@ilan-gold ilan-gold requested a review from flying-sheep January 9, 2025 18:45
@ilan-gold ilan-gold changed the title (fix): upper bound zarr at runtime (fix): upper bound zarr at runtime (and in docs) Jan 9, 2025
@ilan-gold ilan-gold changed the title (fix): upper bound zarr at runtime (and in docs) (fix): upper bound zarr at runtime (and in docs + benchmark) Jan 9, 2025
@ilan-gold
Copy link
Contributor Author

ilan-gold commented Jan 9, 2025

@flying-sheep Is there a way to cross reference old versions of docs? Or are we just going to have to take it on the chin here until we upgrade zarr?

Right, the intersphinx mapping is not automatic

Copy link
Member

Choose a reason for hiding this comment

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

maybe we should add a zarr extra that people can rely on instead of having to maintain their own pin.

then they can just specify anndata[zarr] and get the newest supported version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This struck me as a minor version change. I think this solution also would be necessary anyway to give a helpful message.

Copy link
Member

Choose a reason for hiding this comment

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

I think the raise RuntimeError you added is the most helpful thing for users here. Although maybe it should be an ImportError, it’s not like users can make a change to the runtime parameters to fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thought crossed my mind, I can update

Copy link
Contributor Author

@ilan-gold ilan-gold Jan 10, 2025

Choose a reason for hiding this comment

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

Also another reason not to add a dep now is that I have this slated for 0.12 anyway with the xarray object.

@flying-sheep flying-sheep mentioned this pull request Jan 10, 2025
3 tasks
Copy link

codecov bot commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.63%. Comparing base (a8c62e8) to head (3a163f3).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/anndata/compat/__init__.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1819      +/-   ##
==========================================
- Coverage   87.10%   84.63%   -2.48%     
==========================================
  Files          40       40              
  Lines        6135     6138       +3     
==========================================
- Hits         5344     5195     -149     
- Misses        791      943     +152     
Files with missing lines Coverage Δ
src/anndata/compat/__init__.py 80.97% <66.66%> (-3.87%) ⬇️

... and 7 files with indirect coverage changes

@ilan-gold ilan-gold enabled auto-merge (squash) January 10, 2025 15:52
@ilan-gold ilan-gold merged commit 1bf63a0 into main Jan 10, 2025
13 checks passed
@ilan-gold ilan-gold deleted the ig/zarr_compat branch January 10, 2025 16:13
meeseeksmachine pushed a commit to meeseeksmachine/anndata that referenced this pull request Jan 10, 2025
flying-sheep pushed a commit that referenced this pull request Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants