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

Improve documentation for developers #1419

Closed
WeilerP opened this issue Sep 16, 2020 · 12 comments
Closed

Improve documentation for developers #1419

WeilerP opened this issue Sep 16, 2020 · 12 comments

Comments

@WeilerP
Copy link
Contributor

WeilerP commented Sep 16, 2020

Description

For an easier start of potential contributors, it might be helpful to add a file such as requirements-dev.txt including all packages required to e.g. run unit tests locally. Otherwise, several packages (e.g. leidenalg, louvain, scikit-misc, harmonypy, python-igraph) have to be installed manually which is rather tedious:

-r requirements.txt

harmonypy
leidenalg
louvain
scitkit-misc
python-igraph

The wheels to install python-igraph under Windows can, for example, be found here.

@WeilerP
Copy link
Contributor Author

WeilerP commented Sep 17, 2020

Realised this functionality is already available via pip install ".[dev]". May be good to mention somewhere, though.

@WeilerP WeilerP changed the title Add requirements file for developers Improve documentation for developers Sep 22, 2020
@fidelram
Copy link
Collaborator

This is indeed very valuable information.

Do you mind adding a PR updating https://github.com/theislab/scanpy/blob/master/docs/installation.rst ?

@WeilerP
Copy link
Contributor Author

WeilerP commented Sep 28, 2020

Yes, no problem. I'll update it in a bit.

@WeilerP
Copy link
Contributor Author

WeilerP commented Oct 3, 2020

I had a look at scanpy's setup file setup.py and realised that running

pip install -e .
pip install ".[dev]"

does neither install all packages used within Scanpy's code base nor packages required for documentation or testing.

IMO, these packages should be installed in a developer installation as they are all part of the development cycle. Adding a file requirements-dev.txt including all needed packages would be an option to allow for an easy developer installation via

pip install -e .
pip install -r requirements-dev.txt

Any thoughts on this?

@WeilerP
Copy link
Contributor Author

WeilerP commented Oct 3, 2020

Were there every any thoughts or discussions on adding pre-commit hooks? This would, for example, automatically enforce the black coding style prior to committing new changes.

The .pre-commit-hooks.yaml file could, for example, look as follows:

# .pre-commit-hooks.yaml
repos:
-   repo: https://github.com/ambv/black
    rev: stable
    hooks:
    - id: black

@giovp
Copy link
Member

giovp commented Feb 10, 2021

can we follow keep discussing on #1563 ?
@WeilerP not sure if you ever submitted that PR, but otherwise we can close this

@WeilerP
Copy link
Contributor Author

WeilerP commented Feb 10, 2021

Sorry, no, I didn't open a PR since I hadn't heard back regarding above comments. Fine to close it in favour of #1563, although it only concerns pre-commit hooks, right? The original idea of this issue was to make the dev installation easier, i.e. installing all required packages to run the full code base, tests etc. This currently doesn't happen when installing through pip install ".[dev]".

@giovp
Copy link
Member

giovp commented Feb 10, 2021

that's a very good point yes, I know there is also #1527 underway, can you guys comment on this @flying-sheep @ivirshup cause I'm not sure timing wise how is it going ?

@ivirshup
Copy link
Member

I think #1527 is coming along nicely. It seems to mostly work now. It'd be great if some other team members could try it out and see if it works for them!

@WeilerP
Copy link
Contributor Author

WeilerP commented Feb 11, 2021

As far as I can tell, #1527 still doesn't install all packages in a dev installation required to run the entire code base and tests.

@ivirshup
Copy link
Member

flit install -s by default would install everything, passing --deps=develop actually leads to fewer things being installed. I think this is weird behavior, but it's the way flit works.

@ivirshup
Copy link
Member

I think this is has been covered by the "Contributing" section of the docs.

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

No branches or pull requests

5 participants