lina-usc / pylossless

🧠 EEG Processing pipeline that annotates continuous data
https://pylossless.readthedocs.io/en/latest/
MIT License
20 stars 8 forks source link

Review the setup.py #34

Closed christian-oreilly closed 1 year ago

christian-oreilly commented 1 year ago

Currently, `pip install ./pylossless” will not install the required dependency because the setup.py do not use that file… nor is it clear that it should (https://stackoverflow.com/a/33685899/1825043). Right now, dash is not part of the setup.py dependencies, so it crashes. We could update the missing package to the setup.py, but I must admit that I don’t like this duplication, and I am not sure what to think of the discussion in the post I shared. I think the approach for the setup need to be revised for consistency and for adherence to best practices for package deployment.

Also, pip install pylossless should installs the automated pipeline, and pip install pylossles[dash] (or pylossless[qc]) should install the dependencies for the dashboard. This way folks who just want to use the automated pipeline don't need all the dependencies.

scott-huberty commented 1 year ago

I think @jadesjardins was having issues installing pylossless that might be related to this.

We should probably try to look into this soon.

christian-oreilly commented 1 year ago

What about creating a "release 0.1" issue with a list of checkboxes (https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/about-task-lists) with all the things that we want to have done for this release and going through them one-by-one? I think this issue should be on that list since having a robust setup.py is important for distribution.

scott-huberty commented 1 year ago

What about creating a "release 0.1" issue with a list of checkboxes (https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/about-task-lists) with all the things that we want to have done for this release and going through them one-by-one? I think this issue should be on that list since having a robust setup.py is important for distribution.

absolutely!!!

Andesha commented 1 year ago

I was poking around with the pipeline today and ran into this issue.

I like @christian-oreilly's idea of the multiple setup modes.

However there's a typo in your setup.py right now with "pyaml" instead of "pyyaml".

This will likely get things going for users doing just a base install.

scott-huberty commented 1 year ago

Thanks @Andesha - It's great to have others trying this out so we can iron out these problems before an initial release. I am iterating through a few issues that should hopefully be fixed this week - though I haven't gotten to this one yet.

Did you fix the typo to setup.py and get it to work on your local machine? If so, would you be willing to open a PR?

If not np I'll get to it hopefully soon.

christian-oreilly commented 1 year ago

Thanks for catching this @Andesha. That is interesting... I made a Colab example not long back that was git-clone / pip-install procedure and running the dash app. I am surprised it did not catch this. We must have introduce this bug later, merging another branch or something of the sort.

w/r to multiple setup modes, do you mean something like pip install pylossless[dash], pip install pylossless[plain], etc? We did talk about implementing something like that, but did not yet seriously working on making a proper install script. @scott-huberty is working on CI, so I guess that will soon come in the forefront...

Andesha commented 1 year ago

I'll open a PR as I think I can get it working

In my own understanding btw:

I haven't done multiple setup modes like that before personally, but yes! seems great

scott-huberty commented 1 year ago

Awesome!

re: @christian-oreilly , yeah, I was thinking that we specify the dash requirements as an extra, i.e. pip install pylossless for the base requirements of the automated pipleine, and pip install pylossless[qcr] (or pylossless[dash], or whatever is preferred) for the qc dashboard requirements.

scott-huberty commented 1 year ago

I think this is what we want:

from setuptools import setup
from pathlib import Path

install_requires = ['numpy', 'EDFlib-Python', 'mne', 'mne_bids', 'pandas',
                    'xarray', 'scipy', 'mne_icalabel', 'pyyaml',
                    'scikit-learn']

extras = {'dash': 'requirements_qc.txt',
          'test': 'requirements_testing.txt'}

extras_require = {}
for extra, req_file in extras.items():
    with Path(req_file).open() as file:
        requirements_extra = file.read().splitlines()
    extras_require[extra] = requirements_extra

setup(
    name='pylossless',
    version="0.0.1",
    description='Python port of EEG-IP-L pipeline for preprocessing EEG.',
    python_requires='>=3.5',
    author="Scott Huberty",
    author_email='seh33@uw.edu',
    url='https://github.com/lina-usc/pylossless',
    packages=['pylossless'],
    install_requires=install_requires,
    extras_requires=extras_require,
    include_package_data=True,
    entry_points={"console_scripts": ["pylossless_qc=pylossless.dash.app:main"]}
)

pip install pylossless would install the base requirements

pip install pylossless[dash] would install the base requirements + dashboard requirments ditto for pip install pylossless[test] in regard to testing requirements.