search

lina-usc / pylossless

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

Update edf dependency #161

Closed scott-huberty closed 1 month ago

scott-huberty commented 1 month ago

This fixes a CI failure first reported in #159 (see https://github.com/lina-usc/pylossless/actions/runs/9830889740/job/27231055697?pr=159)

There are a couple of ways we can go about this.

  1. Add a lower cap on the MNE version in our requirements.txt file, i.e. mne>=1.7. This assures that people don't have an older version of MNE installed that will try to use edflib-python.
  2. Follow SPEC0 and officially support all MNE versions that were released within the last 2 years (so that goes back to mne 1.4). Thus, we would need to add code that checks what MNE version the user has installed. If it is < 1.7, check if edflib-python is installed. If it is >1.7, check if edfio is installed, etc.

Personally I vote for 1, because I think the maintenance burden of 2 is currently too much for this project. Also, MNE only officially supports their latest stable release, so I don't want to support people using older versions that MNE doesn't even support themselves.

@Andesha @jadesjardins @christian-oreilly What do you think?

scott-huberty commented 1 month ago

I'm self merging this PR so that #159 is not blocked, but I will repost my comment in a separate GH ticket so that we can discuss

christian-oreilly commented 1 month ago

Personally I vote for 1, because I think the maintenance burden of 2 is currently too much for this project. Also, MNE only officially supports their latest stable release, so I don't want to support people using older versions that MNE doesn't even support themselves.

@Andesha @jadesjardins @christian-oreilly What do you think?

SPEC0 is a nice initiative. I did not know about it. In general, I favor the requirements being as flexible as possible to avoid creating dependency conflict in downstream projects but I am good with 1 too because this is a relatively young project so I think we need to keep the maintenance burden relatively low until there is more buy-in from the community.

scott-huberty commented 1 month ago

SPEC0 is a nice initiative. I did not know about it. In general, I favor the requirements being as flexible as possible to avoid creating dependency conflict in downstream projects but I am good with 1 too because this is a relatively young project so I think we need to keep the maintenance burden relatively low until there is more buy-in from the community.

@christian-oreilly one potential middle ground is that, assuming #159 get merged and users no longer have to save their files as EDF (they can save them to any format mne_bids.write_raw_bids supports; brainvision, fif etc.) - then I think there's a case to be made to drop the hard dependency on edifo in this project:

Then, If the user wants to export to EDF:

Basically we let MNE handle communicating their dependencies.

The downside is that it could cause an annoyance if a user accepts our pipeline defaults, and processes their whole only for it to fail at the end because they don't have the ability to write EDF. We could try to add extra warnings in the documentation to communicate this.

What do you think?

scott-huberty commented 1 month ago

Sorry.. I just saw your response in #159 . I'll respond there