(chore): derive CI matrix from hatch env #3607
Conversation
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
|
|
||
| [[envs.hatch-test.matrix]] | ||
| deps = [ "stable", "full", "pre", "min" ] | ||
| deps = [ "stable", "pre", "min-vers", "min-extras" ] |
There was a problem hiding this comment.
I think this should be changed to minimal-dependencies because if I am not mistaken, that's what it's testing for. Right now it reads like "minimum version of the extras" but that's actually what min-vers is for.
| harmony = [ "harmonypy" ] # Harmony dataset integration | ||
| scanorama = [ "scanorama" ] # Scanorama dataset integration | ||
| scrublet = [ "scikit-image>=0.20" ] # Doublet detection with automatic thresholds | ||
| louvain = [ "igraph", "louvain>=0.8.2" ] # Louvain community detection |
There was a problem hiding this comment.
Are we allowed to bump minimum versions like this on the minor release?
There was a problem hiding this comment.
We’re not, this is the minimum possible version that actually works. Anyone who tried running scanpy with e.g. louvain 0.8.0 would have run into an error. Also bumping patch versions in a patch version is fine, no? Just fixes.
This PR changes the min-deps script so it doesn’t try to do fuzzy matching of the “patch” version. (see below on why I’m using scare quotes). We added a .* to the end of the version, i.e. we had an actually functional version of what Ilia Kats tried in the anndata PR. His version didn’t work, because it used ~= together with adding a .0 but it didn’t guard against doing that multiple times, therefore e.g. making thing>=1.5 into thing~=1.5.0.0.0.0 or so, which just means thing==1.5 (~= in Python is not a semver-aware operator like in JS, it just transforms =~stuff.x into >=stuff.(x+1), <(stuff+1) no matter how many dots stuff has)
I was always against this fuzzy matching: it meant
- that we can’t specify the actual minimum version for which we wanted (as said,
louvain>=0.8is a lie, only>=0.8.2is functional) - we’re adding special magical semantics based on the number of dots to our version boundaries which one would need to know to meaningfully change the boundaries
The only way fuzzy matching could work is if we maintained a list of dependencies that we know actually use semver and use it. But if we’d do that in min-vers.py, we wouldn’t actually test if our minimum version bounds are actually meaningful, so I don’t like that either.
The perfect solution would be:
- what I do in this PR
- we add a dependabot or so config that auto-bumps just the patch versions of known semver dependencies regularly
That’d mean our minimum version bounds are actually meaningful, but we’d still make sure to use the latest patches
There was a problem hiding this comment.
Anyone who tried running scanpy with e.g. louvain 0.8.0 would have run into an error. Also bumping patch versions in a patch version is fine, no? Just fixes.
In this case, it's a minor version but if this is a no-change due to the non-functioning status of louvain, then we're fine. So since it was a minor version change (previously 0.6.0), why was that not working? Was not properly tested because of fuzzy matching? I don't think the two are related, so would be good to undersand here.
therefore e.g. making thing>=1.5 into thing~=1.5.0.0.0.0 or so, which just means thing==1.5
I noticed this as well once you brought up the potential pitfalls of what he did, thanks for that.
we add a dependabot
Neutral on this, but would support if we add some sort of auto-merge strategy and/or automatically merge if tests pass. With JS projects I'm always hesitant to merge given the complexity of literal user-facing graphical applications (i.e., button size changes or something).
| "scanpy[magic]", | ||
| "scanpy[skmisc]", | ||
| "scanpy[harmony]", | ||
| "scanpy[scanorama]", |
There was a problem hiding this comment.
I read the description of this PR:
The only regression is that we no longer test scanorama, because it depends on annoy, which hasn’t been updated in years and fails to build on my system.
Presumably this is coming from spotify/annoy#680. Can we put a condition on this for 3.13? Separately, I would be curious how this has been working the past few months at all. I semi-recently downloaded the package no issue. On first glance, for example, https://github.com/scverse/scanpy/actions/runs/15063089888/job/42341857082 has annoy.
There was a problem hiding this comment.
I'm not saying I actually know what is going on on your system but it seems like perhaps we could use https://packaging.python.org/en/latest/specifications/dependency-specifiers/#dependency-specifiers as a middle ground between completely removing and leaving something in that breaks your system.
There was a problem hiding this comment.
Yes, we should fix this before this PR gets merged.
scanorama→annoy is the only dependency that requires building a native package on my system, I think, and annoy doesn’t seem to be very actively maintained, that’s why I thought we should evaluate this a little.
Co-authored-by: Ilan Gold <[email protected]>
See https://github.com/scverse/cookiecutter-scverse/blob/d4bdfc5ec4c5029aebf7c0cba65609e20358144d/%7B%7Bcookiecutter.project_name%7D%7D/.github/workflows/test.yaml
This also updates the hatch env matrix to have the following jobs
stable(formerlyfull): run all pretty much tests except external toolspre(unchanged): run tests with pre-release versionsmin-vers(formerlymin): run tests with the minimum possible versionsmin-deps(new): like in anndata, tests if everything imports OK with no optional dependencies installedThe only regression is that we no longer test
scanorama, because it depends onannoy, which hasn’t been updated in years and fails to build on my system.We should probably think of a better strategy for it, but I think running the maximum set of tests that we test online also locally is a good idea.
We also need to check if codecov still works, I’m typing this while tests still run.