simonarvin / eyeloop

EyeLoop is a Python 3-based eye-tracker tailored specifically to dynamic, closed-loop experiments on consumer-grade hardware.
GNU General Public License v3.0
479 stars 68 forks source link

Add setup.py and setuptools #20

Closed kinow closed 3 years ago

kinow commented 3 years ago

Hi!

This pull request adds setuptools to the project, with a simple setup.py (used an existing project I maintain as reference, and the Python docs too to remind me of some syntax and configuration).

I normally use virtual environments, or occasionally Conda environments. And also use the IDE with projects I am reading the code or testing. So my workflow is normally:

python -m venv venv
source venv/bin/activate
(venv) pip install -e . # install editable, so that I can change code and keep debugging in the IDE
(venv) # now anything in this virtual environment is self-contained, even scripts like `eyeloop` would exist only here

The pre-print paper I think mentions eyeloop.py, and the new docs eyeloop/run_eyeloop.py. In this pull request, I used entry points of setuptools.

This is a feature in setuptools that allows us to use an existing Python function in a module (e.g. eyeloop.run_eyeloop:Eyeloop) and map it as a Python script automatically created by setuptools, and added to the $PATH variable when the virtual environment is activated.

Using this feature, users can run simply eyeloop. That should take care of choosing the right interpreter, loading dependencies, and starting the GUI.

Not sure if the metadata is complete. There are many classifiers that can be used to help users finding the project via PYPI. Here's the complete list.

Another advantage of using setuptools, is that once we are able to install with pip install ., it is quite simple to upload it to PYPI too with twine or a similar tool. This way, users would be able to install it via pip install eyeloop or some other similar package name (it's not in use now, might be a good idea to register the module if there are any intention of publishing it with that name).

And furthermore, once published to PYPI, if we upload a sdist (source distribution; you can choose to upload wheel binaries only, or source only, or multiple packages) then a Conda package can be created from that source distribution, and made available in Conda Forge.

I will use it locally for experimenting with Eyeloop as it's convenient for me when playing with the code. But thought perhaps it could be useful in case there is interest in publishing to PYPI, Conda, or just allowing users to use pip/poetry/etc.

Cheers Bruno

ps: updated the docs with what I think would need to be updated, but there are probably other scripts in other folders and other documentation that would need to be updated I think? ps2: only the package eyeloop and the setup.py (and LICENSE too I think) are part of the final package; before a first release to PYPI, someone would need to define what is included. Some projects opt to include examples and tests, others avoid that. No right or wrong I believe.

cfculhane commented 3 years ago

This is an interesting one as I think it's a bit of a style preference. From looking around on the internet (to cherry-pick opinions that agree with mine, of course 😄 ), it seems like a useful separation between requirements.txt and install_requires=[...] is requirements.txt is for development and/or an application, where as install_requires=[...] is for library that is installed and imported into another project.

The python docs cover this fairly well.

The middle approach is to specify the minimum required packages (without versioning information) in install_requires=[...] , and use requirements.txt to cover all use cases and for developing for eyeloop.

Another approach is to read them using parse_requirements like this example

kinow commented 3 years ago

This is an interesting one as I think it's a bit of a style preference. From looking around on the internet (to cherry-pick opinions that agree with mine, of course smile )

:laughing: I probably did the same when preparing the PR description using some links.

it seems like a useful separation between requirements.txt and install_requires=[...] is requirements.txt is for development and/or an application, where as install_requires=[...] is for library that is installed and imported into another project.

That'd be right. I know some projects follow that approach. JupyterHub has setup.py for packaging, requirements.txt read by setup.py, and dev-requirements for extra stuff necessary for testing and development.

The middle approach is to specify the minimum required packages (without versioning information) in install_requires=[...] , and use requirements.txt to cover all use cases and for developing for eyeloop.

Another approach is to read them using parse_requirements like this example

The way I am using in another project, is through the setuptools' extras_requires. With extra groups like all, testing, oracledb, etc.

But I think parsing the requirements.txt would be the best approach here.

kinow commented 3 years ago

The example from the gist was broken in Python 3.8 as the pip module changed. So used the JupyterHub code to read from requirements.txt. Updated Travis CI too. But both on Travis and on my laptop the current test command is failing :disappointed:

simonarvin commented 3 years ago

The example from the gist was broken in Python 3.8 as the pip module changed. So used the JupyterHub code to read from requirements.txt. Updated Travis CI too. But both on Travis and on my laptop the current test command is failing 😞

Hi @kinow, I played around with this branch, adding some checks to detect or prevent errors, but could not figure out why this is failing. On my build, EyeLoop runs successfully, but returns an error code on termination (exited with code 1).

kinow commented 3 years ago

The example from the gist was broken in Python 3.8 as the pip module changed. So used the JupyterHub code to read from requirements.txt. Updated Travis CI too. But both on Travis and on my laptop the current test command is failing disappointed

Hi @kinow, I played around with this branch, adding some checks to detect or prevent errors, but could not figure out why this is failing. On my build, EyeLoop runs successfully, but returns an error code on termination (exited with code 1).

It is really strange. I ran the same code Travis CI is running, using the PyCharm debugger. It crashed consistently when displaying the frame/source 307. So I used traceback module and found that it was due an exception here

So I positioned the code in the editor. Started the test again. And waited until the counter reached 300, and enabled the exception in that line.

Then pressed F8 to let the execution go until the source 307, where the error was supposed to happened.

image

I marked what I think is the problem with a red marker. The image object returned by self.capture.read() is None. Then cv2.cvtColor tries to use it, and raises an exception.

The self.capture was able to read the previous sources. The vid_path value of the self.capture also points to the correct video.

image

So I am guessing that perhaps the version of OpenCV used could be the issue? Maybe they changed their API and return None when there are no more frames to read?

image

Will push a simple fix to check if the value is None and just stop and return. That should fix the issue I think, but someone else would have to validate that that fix doesn't cause any regression.

kinow commented 3 years ago

Actually, the fix was much much simpler @simonarvin . We just had to specify the exit code 0. Otherwise the script apparently exists with 1. I am using that in another project, where we migrated old scripts that did the sys.exit(0), so I never had that problem. #TodayILearned :-)

kinow commented 3 years ago

(but I think the OpenCV importer can be changed a bit to avoid some exceptions, might have another look later, first want to read again about OpenCV to refresh and learn more how to use it)

simonarvin commented 3 years ago

Great work, @kinow! I'll merge this pull request.