search

NeuroTechX / EEG-ExPy

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

ci: fix broken CI #167

Closed ErikBjare closed 2 years ago

ErikBjare commented 2 years ago

@JohnGriffiths please be more careful next time, heh: https://github.com/NeuroTechX/eeg-notebooks/commit/3efcb158f5cb2111072320a67046315789c3101b

TODO

Changes

ErikBjare commented 2 years ago

And @JohnGriffiths why is psychtoolbox even needed? Why is it not in the requirements file? What does the JG_ADD comment mean?

ErikBjare commented 2 years ago

Ah, I assume psychtoolbox was to fix https://discourse.psychopy.org/t/missing-sound-libraries-for-psychopy3-standalone-release-3-2-3-for-win32/9162

However, it was already installed, so it had no effect. Added pygame instead.

Edit: adding pygame also had no effect.

ErikBjare commented 2 years ago

I'd also like to note that the dependency hell that caused this would have been handled by poetry, were we to use it.

ErikBjare commented 2 years ago

Alright, so this PR got a bit messier than first thought. Looks like a bunch of issues arising from having a >2yr old version of psychopy... Hoping psychopy has decent backwards compatibility.

ErikBjare commented 2 years ago

Ugh, apparently psychopy now depends on tobii-research, which is only available for Python 3.8+

Python 3.7 is EOL in ~1 year anyway, might be time to bump it.

ErikBjare commented 2 years ago

Now only macOS is failing, needs something like https://github.com/bambocher/pocketsphinx-python/pull/71

ErikBjare commented 2 years ago

Fixed seaborn>=0.11 compatibility (which dropped tsplot).

Building docs is still failing, but I don't think that's related to the changes.

ErikBjare commented 2 years ago

@JohnGriffiths This PR should now be an overall major improvement, but still some minor stuff needed (fix building docs & building on macOS). That'll have to wait for another PR though.

There may still be bugs or more testing needed, esp with the updated versions of stuff, but it's not sustainable to stay on the old versions anyway (as that'll lead to its own problems) so better get this out of the way.

ErikBjare commented 2 years ago

Merging.

JohnGriffiths commented 2 years ago

Thanks for doing the work on this @ErikBjare . Timing is not great though as we have just launched NTCS this week with e-mail out to thousands of of openbci users and this merge has broken installation.

JohnGriffiths commented 2 years ago

Our Installation instructions currently say to use py37 and install wxpython.

conda create -n "eeg-notebooks" python=3.7 git pip wxpython
conda activate "eeg-notebooks"
git clone https://github.com/NeuroTechX/eeg-notebooks
cd eeg-notebooks
pip install -e .

I am troubleshooting this now but definitely just switching to python=3.8 doesn't work as there's no wxpython for py37.

Trying to recall now why we settled on that wxpython instruction (was definitely an interim fix). We did go round the houses between py37, py38, and py39 last time we tackled this and settled on 3.7.

JohnGriffiths commented 2 years ago

Also important = any installation instruction updates need to be reflected in the docs instructions, and currently we have a build failure in the docs, so can't actually change the documentation instructions.

I have tried several times to resolve this problem but haven't been able to yet. The doc build seems to work fine on all local machines I've tried so a bit stumped.

JohnGriffiths commented 2 years ago

install with py38 is failing on two out of two windows machines for me now. have you tested this? are there extra install instructions?

might need to revert this PR.

@oreHGA @JadinTredup if you got a mo to check on this would be great as is pretty time critical.

ErikBjare commented 2 years ago

I'll jump on it

Sorry about the NTCS launch, did not see the announcement until later!

We can always create a revert commit and merge it back later.

ErikBjare commented 2 years ago

There are wxPython wheels for Python 3.8 here, as detailed in requirements.txt: https://extras.wxpython.org/wxPython4/extras/linux/gtk3/ubuntu-20.04/

ErikBjare commented 2 years ago

@JohnGriffiths I have a revert for this ready to go, just give me the word!

JohnGriffiths commented 2 years ago

Yeah I think revert for now and we can work on the PR as a priority matter

On Sun., Mar. 27, 2022, 1:53 p.m. Erik Bjäreholt, @.***> wrote:

@JohnGriffiths https://github.com/JohnGriffiths I have a revert for this ready to go, just give me the word!

— Reply to this email directly, view it on GitHub https://github.com/NeuroTechX/eeg-notebooks/pull/167#issuecomment-1079983734, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADBEAWNUY6FRKNL6J3EENDVCCVA5ANCNFSM5RTYZBGA . You are receiving this because you were mentioned.Message ID: @.***>