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

We broke scanpy’s tests #1342

Closed
3 tasks done
flying-sheep opened this issue Jan 26, 2024 · 10 comments
Closed
3 tasks done

We broke scanpy’s tests #1342

flying-sheep opened this issue Jan 26, 2024 · 10 comments
Labels

Comments

@flying-sheep
Copy link
Member

flying-sheep commented Jan 26, 2024

Please make sure these conditions are met

  • I have checked that this issue has not already been reported.
  • I have confirmed this bug exists on the latest version of anndata.
  • (optional) I have confirmed this bug exists on the master branch of anndata.

Report

The tests were broken in #1266

pip install anndata  # anndata[testing] doesn‘t exist
import anndata.tests.helpers

Traceback:

ModuleNotFoundError: No module named 'zarr'

The fix is not to add zarr to scanpy’s test dependencies, but to make the zarr import conditional

Afterwards, it’s time to finally fix #646

Versions

anndata 0.10.5
@ivirshup
Copy link
Member

ivirshup commented Jan 26, 2024

I don't know that it's a bug to import from a private module that has optional dependencies (which are specified under anndata[test]) and for that to error.

Seems related to why I had to pin anndata fairly high here: scverse/scanpy#2816

There really is an issue of needing a similar set of helpers, but not wanting these helpers to be public because they're not considered stable.

Also, I really think the immediate answer is to add zarr with a TODO comment to the scanpy test dependencies.

@flying-sheep
Copy link
Member Author

@ilan-gold is on it. Let’s not have zarr feature-creep its way into scanpy’s minimal test deps. It’s the day of hotfixes it seems

@flying-sheep
Copy link
Member Author

Once pytest 8.0 is out, we can finally tackle the whole reorganization:

  1. make the anndata repo use a src layout
  2. create the public test module (anndata.test_utils or anndata.testing or testing.anndata) with an associated extra
  3. have scanpy depend on that
  4. move anndata’s tests out of the package

@ivirshup
Copy link
Member

I'm not against doing a testing module, though I do think we have to be selective about what goes in it and probably need to test it more thoroughly.

I just don't think that this problem neccesarily needs an immediate bug fix release.

@flying-sheep
Copy link
Member Author

flying-sheep commented Jan 26, 2024

I'm not against doing a testing module, though I do think we have to be selective about what goes in it and probably need to test it more thoroughly.

totally! those are all reasons why we should do it!

I just don't think that this problem neccesarily needs an immediate bug fix release.

I don’t like accumulating hacks, I guess. Doing it the other way means we can forget about it, and can’t merge the minimum-deps branch before we have removed zarr again.

@ivirshup
Copy link
Member

Could do a .post release?

I don’t like accumulating hacks, I guess. Doing it the other way means we can forget about it, and can’t merge the minimum-deps branch before we have removed zarr again.

I'm not sure how that PR would be more affected by this?

@ivirshup
Copy link
Member

I've checked the current 0.10.x branch against scanpy, I think a post branch seems appropriate here. I can make one from 08e7b94.

Any objection?

@flying-sheep
Copy link
Member Author

I think I don’t fully understand. If you just mean calling the release .post instead of giving it a new patch version number, sure!

@ivirshup
Copy link
Member

@ivirshup
Copy link
Member

Guess it's just flaky somehow?

https://github.com/scverse/anndata/actions/runs/7670870992/job/20907984190

Anyways, release made

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants