Skip to content

(fix): check xarray installation when using any xarray-dependent code + fix string dtypes with new bound#1991

Merged
ilan-gold merged 21 commits into
mainfrom
ig/error_on_read_elem_lazy
Jun 12, 2025
Merged

(fix): check xarray installation when using any xarray-dependent code + fix string dtypes with new bound#1991
ilan-gold merged 21 commits into
mainfrom
ig/error_on_read_elem_lazy

Conversation

@ilan-gold
Copy link
Copy Markdown
Contributor

@ilan-gold ilan-gold commented May 16, 2025 •
edited
Loading

  • Closes: QOL issue
  • Tests added
  • Release note added (or unnecessary)

@ilan-gold ilan-gold added this to the 0.12.0 milestone May 16, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.35%. Comparing base (fc892b3) to head (f57e571).
⚠️ Report is 44 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1991      +/-   ##
==========================================
- Coverage   87.14%   85.35%   -1.80%     
==========================================
  Files          46       46              
  Lines        6986     7005      +19     
==========================================
- Hits         6088     5979     -109     
- Misses        898     1026     +128
Files with missing lines Coverage Δ
src/anndata/_core/xarray.py 98.47% <100.00%> (+0.18%) ⬆️
src/anndata/_io/specs/lazy_methods.py 96.12% <100.00%> (+0.07%) ⬆️
src/anndata/experimental/backed/_io.py 96.15% <100.00%> (+5.24%) ⬆️
src/anndata/experimental/backed/_lazy_arrays.py 94.28% <100.00%> (+0.05%) ⬆️
src/anndata/tests/helpers.py 82.91% <100.00%> (-10.24%) ⬇️

... and 7 files with indirect coverage changes

@ilan-gold
Copy link
Copy Markdown
Contributor Author

ilan-gold commented May 20, 2025

The codecov is complaining because we need a similar fix to scverse/scanpy#3607 (comment) where we have "min-deps" i.e., no xarray dask etc.

@ilan-gold ilan-gold requested a review from flying-sheep May 23, 2025 11:08
@ilan-gold ilan-gold changed the title (fix): check xarray installation when reading in a dataframe (fix): check xarray installation when reading in any xarray-dependent code May 23, 2025
@flying-sheep
Copy link
Copy Markdown
Member

flying-sheep commented May 26, 2025

Yeah! Would you do the equivalent PR to scverse/scanpy#3607 or should I?

@ilan-gold
Copy link
Copy Markdown
Contributor Author

ilan-gold commented May 27, 2025

I'm happy to do it @flying-sheep.

ilan-gold
ilan-gold commented Jun 3, 2025
View reviewed changes
Comment thread hatch.toml Outdated
@ilan-gold ilan-gold force-pushed the ig/error_on_read_elem_lazy branch from 118a304 to 1f91433 Compare June 3, 2025 12:53
@ilan-gold ilan-gold changed the title (fix): check xarray installation when reading in any xarray-dependent code (fix): check xarray installation when using any xarray-dependent code Jun 3, 2025
@ilan-gold ilan-gold requested review from flying-sheep and removed request for flying-sheep June 4, 2025 09:23
@ilan-gold
Copy link
Copy Markdown
Contributor Author

ilan-gold commented Jun 4, 2025

The awkward array docs are down (?) so we'll just rerun tomorrow

@flying-sheep

This comment was marked as resolved.

Sign in to view
@ilan-gold
Copy link
Copy Markdown
Contributor Author

ilan-gold commented Jun 12, 2025

@flying-sheep These tests are going to fail now because of an issue with xarray - see #2008 which is now based against this branch for test pass/failure

flying-sheep
flying-sheep requested changes Jun 12, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@flying-sheep flying-sheep left a comment •
edited
Loading

Choose a reason for hiding this comment

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

See #1991 (comment)

also why did you change away from pytestmark = ...? ideally we keep it that way, so pytest can collect all tests and then just skips the ones it can't run.

in scanpy's test suite, I'm pretty sure pytest is able to collect all tests always.

Comment thread src/anndata/experimental/backed/_io.py Outdated
Comment thread tests/test_concatenate.py Outdated
@ilan-gold
Copy link
Copy Markdown
Contributor Author

ilan-gold commented Jun 12, 2025

@flying-sheep https://github.com/scverse/anndata/actions/runs/15609690266/job/43967402083?pr=1991 was the reason for importorskip. I don't want params to be run. Is there a way around this? importorskip was my solution

@flying-sheep
Copy link
Copy Markdown
Member

flying-sheep commented Jun 12, 2025

As said, it's not that important. You could just defer these parameters with lambda: XDataset(...) though, so it wouldn't be hard either.

@ilan-gold
Copy link
Copy Markdown
Contributor Author

ilan-gold commented Jun 12, 2025

You could just defer these parameters with lambda: XDataset(...) though

I am not a huge fan of this paradigm so I will revert the change then.

@ilan-gold ilan-gold requested review from flying-sheep and removed request for flying-sheep June 12, 2025 14:46
flying-sheep
flying-sheep approved these changes Jun 12, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@flying-sheep flying-sheep left a comment

Choose a reason for hiding this comment

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

looks good, but why did you make everything into importorskip in d2be64e instead of just where necessary?

ilan-gold added 2 commits June 12, 2025 17:17
@ilan-gold
(fix): go back to pytestmark where fixtures not created
60067f8
@ilan-gold
(fix): new xarray bound + string fix (#2008)
f57e571 
* (fix): xarray bound

* (chore): test pre against main

* (fix): handle string dtype

* (fix): categorical dtype access

* (fix): remove numpy2 lower bound

* (fix): convert to pre-install deps file

* (chore): re-locate comment

* (chore): add comment

* (chore): relnote
@ilan-gold ilan-gold enabled auto-merge (squash) June 12, 2025 16:16
@ilan-gold ilan-gold changed the title (fix): check xarray installation when using any xarray-dependent code (fix): check xarray installation when using any xarray-dependent code + fix string dtypes with new bound Jun 12, 2025
@ilan-gold ilan-gold merged commit a6123c0 into main Jun 12, 2025
16 checks passed
@ilan-gold ilan-gold deleted the ig/error_on_read_elem_lazy branch June 12, 2025 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@flying-sheep flying-sheep flying-sheep approved these changes

Assignees

No one assigned

Labels

skip-gpu-ci

Projects

None yet

Milestone

0.12.0

Development

Successfully merging this pull request may close these issues.

2 participants

@ilan-gold @flying-sheep