search

NeuroTechX / EEG-ExPy

EEG Experiments in Python
https://neurotechx.github.io/EEG-ExPy/
BSD 3-Clause "New" or "Revised" License
437 stars 124 forks source link

fix: support Python 3.8 (and 3.9 on Linux) #32

Closed ErikBjare closed 3 years ago

ErikBjare commented 3 years ago

Depends on #31

Fixes #29, Fixes #50

Based on a similar solution in: https://github.com/ActivityWatch/aw-watcher-afk/pull/44

ErikBjare commented 3 years ago

Looks like requirements.txt doesn't accept user-supplied wheels for dependencies with pypi entries.

Another reason to look into #22 I guess.

ErikBjare commented 3 years ago

Might just have been the fact that wheels for Python 3.6 weren't available, bumping CI Python version to check if that's the case.

ErikBjare commented 3 years ago

CI passed, ready to merge (after #31) :tada:

JohnGriffiths commented 3 years ago

Just want to check: do we think the user-supplied wheels is the best option here, or is the pywinhook issue largely solved by moving to python 3.7?

ErikBjare commented 3 years ago

@JohnGriffiths From a second look at https://pypi.org/project/pyWinhook/#files it seems like there exists pywinhook wheels on PyPI for Python 3.7 and 3.8 (news to me), but indeed not for 3.9.

Since there are wheels available for 3.9 (here: https://www.lfd.uci.edu/~gohlke/pythonlibs/), we might want to change this PR to only supply prebuilt wheels for Python 3.9 (and maybe for 3.6 since that's not on PyPI either, but we'll probably want to drop 3.6 support soon anyway).

Edit: It should be added that I intend to resolve issues with newer Python versions, as I've laid out in #50

ErikBjare commented 3 years ago

Supports Python 3.9 on Linux now, but one remaining issue with tables 3.6.1 on macOS and Windows where wheels are missing and requires HDF5 headers to be available (see #50)

ErikBjare commented 3 years ago

Looks like pyriemann broke with the latest scikit-learn, submitted a fix: https://github.com/alexandrebarachant/pyRiemann/pull/93

JohnGriffiths commented 3 years ago

I'm OK with merging this.

But shlukd we be e currently waiting for the pyriemann fix PRs to be accepted before doing so?

ErikBjare commented 3 years ago

If so, we'll need to lower the sklearn constraint again, and leave Python 3.9 support for another PR.

I'd suggest we just wait for it to get merged (or if it takes too long, fork pyriemann ourselves and switch to using that).