update readme with contributing instructions

[akhanf]

copied from snakebids

[jordandekraker]

General development comment:
I'm still testing out poetry (local machine) and pip install (graham), but as i'm going through this i also wanted to note that we're still using autotop_deps-v0.4.1.simg to handle quite a few dependencies. Is the general idea to transition away from using this and have most dependencies handled by python instead? Otherwise maybe we could consider adding python packages to that simg instead (which would also version lock them).

[akhanf]

Yes we are still using that container for all the non-python dependencies. But we are always going to need containers if we have non-python dependencies (unless we re-implement things in python), so no easy way around that..

[akhanf]

however one thing we can do to simplify python dependencies is to specify python dependencies on a rule-by-rule basis, instead of for the entire workflow. Conventionally with snakemake would use conda envs for that, but we haven't been doing that because conda isn't supported on graham. There is now a virtualenv alternative for this with https://github.com/pvandyken/snakeboost so that is something we can start to do in the future.. Probably could just have a couple of different envs, one for nnunet, and one for everything else (nilearn,nibabel,pandas etc)

[jordandekraker]

Re: venvs for rules, I'm not sure that would be much simpler, but maybe.
One concern I have is that poetry places things in ~/.local and ~/.cache. On McGill's and Julich's compute clusters this is annoying because ~ has only very limited space (you're meant to use project-specific shared storage space instead). I'm sure I can make this work with poetry, but its fairly cumbersome.

I'm also still running into a bug in installation:
poetry install worked
poetry run poe setup returns Error: Unrecognised task 'setup'

In my mind, it might be simpler for developers to work inside of a singularity shell where all python and non-python dependencies are present already.

[akhanf]

Re: venvs for rules, I'm not sure that would be much simpler, but maybe.

Rule-based venvs would be simpler in that the workflow would create them automatically and have them defined, instead of having them all in the pyproject toml file.. E.g. if all the rules had venvs/condaenvs then the only python dependencies hippunfold would have is snakebids+snakemake, instead of the long list of dependencies..

One concern I have is that poetry places things in ~/.local and ~/.cache. On McGill's and Julich's compute clusters this is annoying because ~ has only very limited space (you're meant to use project-specific shared storage space instead). I'm sure I can make this work with poetry, but its fairly cumbersome.

Ah the location of those folders is customizable with poetry config. You just need to set this once on the system you're on, and it uses that globally.

I'm also still running into a bug in installation: poetry install worked poetry run poe setup returns Error: Unrecognised task 'setup'

poetry run poe setup actually might be an error in the instructions themselves -- I copied the instructions from another project but may have changed things.. Will need to fix those instructions. Using poe is not a required though, all those quality checks and tests will be done when committed to github anyways.

In my mind, it might be simpler for developers to work inside of a singularity shell where all python and non-python dependencies are present already.

Yes having a singularity-based development environment would work too, if the developer doesn't need to update project dependencies, since the containers are read-only (unless a sandbox is used).. Could be done with the current docker://khanlab/hippunfold:latest containers (all the python dependencies are installed there), we just don't have instructions for how to do that (those instructions maybe could be as easy as just singularity shell, and working with a cloned repository). I haven't been doing that because of the large weight of the containers.. My preferred development environment now (on a local machine) is just to clone the repo, poetry install, and then ready to go (not much different from earlier with clone, then pip install -e). I use snakemake profiles to avoid the need to re-download containers (ie to set the singularity folder implicitly).

[jordandekraker]

Yeah makes sense. I have things working on graham so I can continue testing stuff there. Still having problems executing with poetry, but I'm fine to leave it for now and maybe if I do want to do some local testing I'll try it out inside a singularity container.

At some point we should add these notes to the readthedocs too. We could stick to just containerized version for the installation page and then maybe add a page for contributors?

[akhanf]

Yeah the instructions for contributors will be a separate section from the install instructions (which remain the same). Right now this PR is updating the readme, which gets loaded into the readthedocs main page, before merging this in I'll update the instructions and perhaps move them to a separate contributors page.

[jordandekraker]

I'm still having problems with poetry install. In a clean environment, with poetry installed, poetry install hippunfold works but the only poe configured tasks are quality_check and quality_fix. When i try to run hippunfold from a different directory, workflow.basedir is not correct.

A few other notes:
Maybe good to mention: poetry must be installed with python >=3.7, and poetry installs things in ~/.cache, but can be moved with poetry config cache-dir <new dir>
We should mention this point about linting: #148 (comment)

[akhanf]

@jordandekraker have a look at the readme instructions now and let me know if you're still having problems, especially with the workflow.basedir

[akhanf]

We should also update the enumerated steps in the readme (I think still referring to niftynet), and maybe replace the figure with the one from the preprint.. Will try to do this tomorrow if I have a chance

[jordandekraker]

Agreed, this is much clearer and working for me now!

Yeah I was thinking of updating the workflow summary too. Would be nice to add in some surfaces including the DG as well now that we have it! I also thought we should include a hyperlink to readthedocs, since it seems not everybody notices the badge.

[akhanf]

yes agreed!
We can actually move this to #150 and close this PR, as I've made some other doc changes there (shift to markdown instead of RST)