mad-lab-fau / BioPsyKit

A Python package for the analysis of biopsychological data.
MIT License
39 stars 8 forks source link

[JOSS Review] Installation #13

Closed espenhgn closed 2 years ago

espenhgn commented 2 years ago

This project's README suggests Poetry as the main means of installation of this package and dependencies to very specific versions. This is an issue on my machine, an M1 macbook pro, as not every dependency is readily available from PyPI, mainly scipy v1.7.1. As of now, only MacOS x86_64 wheels are provided. Arm64 builds are also not yet available from unofficial sources like conda-forge. Building such dependencies from source may be an issue for many users. Tried installing BioPsyKit now in a conda environment (using the miniforge python distribution) and in a virtuelenv using packages installed via macports. The only way I could get the different example notebooks to run was to add src to PYTHONPATH and install the closest matching dependencies manually in the python environment I set up.

There is nothing in this package that should prevent use of somewhat older (and future) versions of BioPsyKit's dependencies like scipy.

As the pyproject.toml file is provided, I think the README file could mention pip install . as an installation option without the need to install Poetry.

Another minor issue with the README: cd biopsykit should be cd BioPsyKit in the installation instructions.

Linked issue: https://github.com/openjournals/joss-reviews/issues/3702

richrobe commented 2 years ago

Thank you for the hints!

Regarding the dependencies: biopsykit uses pingouin as a dependency. The latest version of pingouin (0.4.0) brought some API changes and some important bugfixes for statistical analysis which is why it was recommended to upgrade to the lastest version of pingouin. This version, however, requires scipy 1.7+. We downgraded the minimal scipy requirement to 1.7.0 in e380e22f114ef1e8b529ab797527bdf7a2f98f13, but I'm afraid we can't go any lower than that. Would that help for the installation of biopsykit on Apple Silicon M1 devices?

richrobe commented 2 years ago

Regarding the installation: We added the possibility to install a local copy of the package via pip install . in ec48542186a809444f17df54e1eb86c7aeb66b0d.

espenhgn commented 2 years ago

Hi @richrobe ;

Scipy at v1.7.+ should be ok. Arm64 builds for v1.7.1 has also been made available via conda-forge since I opened this issue. Anyway, this time I had better luck installing BioPsyKit (SHA:f97f3ce9f8cc4d8d976715a738d069532efae3df) in a new conda environment by fetching only a subset of packages from conda-forge (omitting building scipy/sklearn etc. from source altogether):

conda create -n biopsykit python=3 pip scipy scikit-learn==0.24.2 statsmodels==0.12.2 poetry
conda activate biopsykit
poetry install
poetry install -E mne -E jupyter 
...

Then I only encountered a couple of issues with the different poetry tasks:

poetry run doit docs
...
Notebook error:
PandocMissing in examples/_notebooks/CFT_Example.ipynb:
Pandoc wasn't found.
Please check that pandoc is installed:
https://pandoc.org/installing.html
make: *** [html] Error 2

Which imply that Pandoc (pandoc.org) is a missing dependency for this project. I installed the tool via conda:

conda install pandoc

which took care of the above issue. The docs at /docs/_build/index.html appears correct.

Finally I encountered some issue with the update_version task running:

poetry run doit update_version
.  update_version
TaskError - taskid:update_version
PythonAction Error
Traceback (most recent call last):
  File "/Users/espenhagen/miniforge3/envs/biopsykit/lib/python3.9/site-packages/doit/action.py", line 437, in execute
    returned_value = self.py_callable(*self.args, **kwargs)
  File "/Users/espenhagen/Repositories/BioPsyKit/dodo.py", line 79, in update_version
    subprocess.run(["poetry", "version", version], shell=False, check=True)
  File "/Users/espenhagen/miniforge3/envs/biopsykit/lib/python3.9/subprocess.py", line 505, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/Users/espenhagen/miniforge3/envs/biopsykit/lib/python3.9/subprocess.py", line 951, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/Users/espenhagen/miniforge3/envs/biopsykit/lib/python3.9/subprocess.py", line 1754, in _execute_child
    self.pid = _posixsubprocess.fork_exec(
TypeError: expected str, bytes or os.PathLike object, not NoneType

########################################
update_version <stdout>

Here, it seems like the README should state that the new version number must be stated, like poetry run doit update_version -v 0.3.2 that's all.

richrobe commented 2 years ago

Hi @espenhgn , thanks for the hint! It's weird that it is complaining about pandoc being a missing dependency but not when building the documentation on read-the-docs. Just to be safe, I added pandoc as dependency.

richrobe commented 2 years ago

Regarding the update_version task for doit, you can simply pass the type of version you want to bump (e.g., doit update_version -v patch to bump the patch version) instead of explicitly passing the version number (which of course also works).

I updated the instructions in the Contributing Guide.

espenhgn commented 2 years ago

Hi @espenhgn , thanks for the hint! It's weird that it is complaining about pandoc being a missing dependency but not when building the documentation on read-the-docs. Just to be safe, I added pandoc as dependency.

Pandoc is likely included in the VM that readthedocs use for their builds -- I assume it is a fairly common dependency. I'm not sure if that particular VM recipe is publicly available anywhere though. As for adding pandoc to the pyproject.toml file, are you sure if this will actually install the pandoc binaries? Seems like https://pypi.org/project/pandoc/ do not in my limited testing. What is needed for your documentation builds is the binary pandoc somewhere in PATH, not necessarily a pandoc python module.

espenhgn commented 2 years ago

Regarding the update_version task for doit, you can simply pass the type of version you want to bump (e.g., doit update_version -v patch to bump the patch version) instead of explicitly passing the version number (which of course also works).

I updated the instructions in the Contributing Guide.

Sounds good! Btw., I spotted a small typo in the Contributing guide: petry run doit -v 0 which should be poetry run doit -v 0

richrobe commented 2 years ago

Pandoc is likely included in the VM that readthedocs use for their builds -- I assume it is a fairly common dependency. I'm not sure if that particular VM recipe is publicly available anywhere though. As for adding pandoc to the pyproject.toml file, are you sure if this will actually install the pandoc binaries? Seems like https://pypi.org/project/pandoc/ do not in my limited testing. What is needed for your documentation builds is the binary pandoc somewhere in PATH, not necessarily a pandoc python module.

Yes, you're right! Sorry, I was acting a bit hasty and was assuming that this is a simple dependency issue. In fact, you need to manually install pandoc (e.g., using brew on macOS) before building the documentation locally. I added the information to the README and to the Contributing Guidelines in 46bbc9d4f3840e1c9ff225de0cbc22cabf3bc32a. Now it should work :-)

espenhgn commented 2 years ago

Great. Then I think all my issues have been resolved :)