SpikeInterface / spikeinterface

A Python-based module for creating flexible and robust spike sorting pipelines.
https://spikeinterface.readthedocs.io
MIT License
533 stars 187 forks source link

Add `dev` section to `pyproject.toml` ? #2235

Closed zm711 closed 11 months ago

zm711 commented 1 year ago

I was resetting up a new env on my laptop and realized that there isn't an easy way to get a bunch of the packages for spikeinterface on the developer side. Since this is really only a couple time thing in general maybe this is overkill. But I could imagine something like:

dev = [
    "pytest",
    "pytest-cov",
    "black",
    "pre-commit",
    "Sphinx==5.1.1",
    "sphinx_rtd_theme==1.0.0",
    "sphinx-gallery",
    "numpydoc",
    "psutil",
]

So for new developers they could do:

pip install -e .[dev]

to get the real meat of the package for testing (for core or add [full,dev] for more). Or if they only want to work on one submodule something like:

pip install -e .[widgets,dev]

This way there is quick easy install for most things (datalad/git-annex are a little too OS specific to let us easily get those). This would get the main testing tools (pytest + cov), the linting tools (black + pre-commit), and the basic doc tools (sphinx + numpydoc), so that a user doesn't have to do:

pip install -e .[full,test_core,test,docs]
pip install black
pip install pre-commit

Again making the environment really only happens once (in a while) so maybe not worth the effort, but might make it easier for new developers. I'm curious what people think. Although if this is super contentious this is not a hill I would die on. If totally supported I'd be happy to do a PR for this, but since I just remade my env I wouldn't even need to use this.

h-mayorquin commented 1 year ago

I am fine with it but is more lines to mantain up to date. Can't that be achieved by a combination of funtions:

Like:

pip install -e .[core_test, doc]

Should almost cover it, doesn't?

zm711 commented 1 year ago

It doesn't cover linting tools and it adds in other things like datalad and some of the more niche packages (like all packages for testing the nn metrics) , which might end up being a mess on someone's computer. But like I said, this only happens once in a while so if the maintenance cost is annoying I don't think this is crucial. More a light convenience to bring in new developers.

h-mayorquin commented 1 year ago

I am fine with it if it makes your life easier. I was just curios on what was missing from a combination of others and why.

JoeZiminski commented 12 months ago

I like this pattern, we do across all our repos for easy install of all testing / linting infrastructure.

JoeZiminski commented 12 months ago

On this subject, I wonder if it makes sense to combine what is currently [test], [test_core], [docs] + linters into a single [dev] section. In general if you are developing you would be expected to be running tests / updating docs anyway.

zm711 commented 11 months ago

I have two concerns with a [dev] section replacing [test][test_core][docs].

1) I don't know how much that will slow down rtd, which for most PRs is the slowest step. I assume very slightly. 2) (and @JoeZiminski as a mac person I've been meaning to ask you) I'm worried about devs on Windows and Mac getting datalad working and downloading some of the test framework automatically may work for linux, but I don't know if combining them all for new devs is the safest option.

My pro/con list is for combining everything is:

pros: few lines of pyproject to maintain, all actions can just use the [dev] rather than random combos of [test,test_core,doc] cons: less flexibility, may make each action take slightly longer

My pro/con list for just adding dev without combining pros: easy way to get dev/dev station up and running for core cons: more lines of code/duplicates

Feel free to add or delete more if you think of it.

alejoe91 commented 11 months ago

It doesn't need to replace the existing ones.

It could be:

dev = [
    "spikeinterface[test]",
    "spikeinterface[test_core]",
    "spikeinterface[docs]",
    "black",
    "pre-commit",
]
JoeZiminski commented 11 months ago

I think that's a nice pattern! it maintains the old flexibiltiy while giving a quick install of all dev requirements.

Good point about datalad @zm711, (I think) in part this issue could be solved by more thorough checks on whether things like datalad / docker / spython are actually installed. Currently the imports are just checked, but for me this has caused problems (e.g. once I had accidently installed docker in my environment and SI tried to run sorting using docker even though docker was not actually installed). So, maybe checking for installs using system calls rather than python packages will solve this problem as even if datalad is installed in python, if datalad is not actually installed on the system no problems will arise.

zm711 commented 11 months ago

dev = [ "spikeinterface[test]", "spikeinterface[test_core]", "spikeinterface[docs]", "black", "pre-commit", ]

I'll test that locally and let you know how it goes

zm711 commented 11 months ago

That wasn't so bad. It's ~ 2gb conda env. Install was a little slow, but not terrible. I would be okay using this strategy.