Adopt cookiecutter-scverse template - #214
Conversation
- Move the package to a src/ layout and build with hatchling - Replace flit/black/flake8 with ruff, biome, prek and the scverse CI/pre-commit stack - Link the repo to the template via .cruft.json and add the standard workflows, dependabot, issue templates and editor/tooling config - Migrate the docs to the template's myst-based Sphinx setup - Raise the minimum Python to 3.12 and type-check the codebase with mypy - Fix bugs surfaced by the migration: tss_enrichment AnnData(dtype=) removal, gene_names/func None-dereferences, non-integer .obsm component index, and a duplicate-key test fixture that silently ran only one case - Use mudata from git until the .h5mu write fix lands in a release Closes #103
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
…verse # Conflicts: # docs/source/changelog.rst # src/muon/_core/preproc.py
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
grst
left a comment
There was a problem hiding this comment.
looks good overall! Can you share to what extent this could be automated with agents? Did you use any specific prompt?
Can be fully automated although this was surely the easiest case. Like I imagine that porting scvi-tools will be more pain. I didn't use a specific long prompt besides more or less "no compromises - full adherence to the template". |
…ispatch - Move the JASPAR data exclusion from .prekignore into the pre-commit config's top-level `exclude` where it belongs - Re-add the `workflow_dispatch` trigger to the release workflow (dropped when adopting the template's release.yaml)
|
I'm currently double checking the docs build which seems to require a few more fixes. @gtca see zulip - please help us by enabling the RTD build on PRs. |
Move the code out of the private `_core`/`_atac`/`_prot`/`_rna` packages so the
public namespaces are genuine importable modules, matching other scverse
packages (e.g. `from muon.pp import neighbors` now works):
- `_core/{preproc,tools,plot,utils,io,config}.py` -> top-level `muon/{pp,tl,pl,utils,io,config}.py`
- `_atac/*` -> `muon/atac/` package (`pp`, `tl`, `pl`, `io`, `utils`, bundled JASPAR data)
- `_prot/*` -> `muon/prot/` package; `_rna/*` -> `muon/rna/`
This lets the documentation use the standard scverse autosummary pattern with the
default templates (no `sys.modules` tricks, no custom autosummary templates).
Public functions newly exposed by the move get one-line docstrings.
- Documentation: adopt the standard scverse autosummary API reference now that submodules are importable (default templates, no custom autosummary), fix the mudata intersphinx URL and add matplotlib/leidenalg, enable Google-style napoleon parsing to match muon's docstrings, and tidy several docstrings. `sphinx-build -W` (nitpicky) now passes with zero warnings. - Remove `muon.tl.louvain` and all louvain references; leiden is kept. - Drop two dead mypy ignores.
The `filepath_h5mu`/`filepath_hdf5` fixtures were module-scoped, so a shared file path was reused across tests. Under the template's parallel test runner (`pytest -n auto`) two workers wrote the same HDF5 file and collided (`BlockingIOError: unable to lock file`). Scope them per function with unique `tmp_path` files instead.
|
@ilia-kats the PR now makes a few more changes that I initially had wanted to avoid
I'd be happy to get this PR in because it touches everything and I just want to finally have this done. If there's minor things we need to improve, I can follow up. I know it's a lot. |
|
Thank you for this. I'll try to have a comprehensive look at it soon, but I have some teaching next week and some other things to do, so it may take a bit. A few things that I've noticed so far:
|
Hmm, I'm in favor of consistency. Maybe we can support both.
No, Louvain support is deprecated in scanpy and will be removed soon. I agree with the separation of concerns of the PRs but Louvain has been dead for years. If you insist, I'll revert this but Louvain should be removed.
Actually, this comes from the template and isn't personal. I can remove it here but then your feedback needs to be directed to the template.
Will do if it's a part of the template. But I don't think that's the case. If we enable it by default in the template, we can happily add it here.
Coming from the template. I'll remove this. It shouldn't be needed here. AnnData 0.13 has improved typing support. Let me know what else you discover. I'll then do all things at once to minimize back and forth. Thanks! |
Per review: the >=0.13 constraint came from the template default and isn't needed by muon.
I'm also in favor of using github releases for discoverability. E.g. in #211 (reply in thread) a user was confused because they couldn't find the release on github. I guess you can still avoid the github UI with
It's an optional feature of the template, but it's not the default. But I think many / most (?) core packages use it, and personally, I'm a fan.
This is also an optional feature of the template and you are asked during |
GH releases make sense when you actually use their features, for example like all the Rust projects attaching pre-buit binaries to releases, which doesn't make sense for us, or having a changelog in the release, which we also don't do. For us, the GH release is essentially a git tag with extra steps. The user's confusion in #211 had two reasons:
|
|
@ilia-kats polite disagree. I was also looking at the Github releases and super confused that they're labeled pre-release and many are missing. This is a super common feature and we're using it across all scverse packages. Consistency is king (across scverse) and I think Github users expect to find all releases properly there. The changelogs can just be copied. I'm fine with the IDE related stuff being removed (say the vscode config) but I have a stronger opinion on Github releases. It's trivial to use, even more so if you use the CLI as Gregor pointed out. It doesn't hurt anyone. |
|
I tend to agree with Lukas here. EDIT: sorry, closing/reopening was a misclick. |
SnapATAC2 is also using a purely tag-based workflow. More generally, creating a GH release also triggers a tag push event, so the existing |
And you can expect me to also look at SnapATAC2 soon 😄 . One package after another - I'm currently also annoying the scvi-tools team and going through them one by one.
I just don't see a reason here to deviate from the template. But anyways, I'm waiting for your review and then we can finalize this PR. |
Adopts the cookiecutter-scverse template (Closes #103).
src/layout + hatchling build; ruff/biome/prek + scverse CI, docs and tooling config; repo linked via.cruft.jsonfor future template syncstss_enrichmentAnnData(dtype=),Nonedereferences,.obsmindex parsing, a duplicate-key test fixture).h5muwrite fix is released; the published wheel keeps a plainmudatadependency