Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update edf dependency #161

Merged
merged 4 commits into from
Jul 22, 2024
Merged

Update edf dependency #161

merged 4 commits into from
Jul 22, 2024

Conversation

scott-huberty
Copy link
Member

@scott-huberty scott-huberty commented Jul 22, 2024

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

  • Since v1.7 (April 2024), MNE uses edfio for writing EDF files. So we no longer should depend on edflib-python, but should use edfio.

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 and others added 4 commits September 19, 2023 15:24
- MNE now depends on EDFIO, not edflib-python
- Don't manually install packages in the CI, instead make sure they are listed in the appropriate requirement txt file
@scott-huberty
Copy link
Member Author

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

@scott-huberty scott-huberty merged commit 46c7afb into lina-usc:main Jul 22, 2024
3 checks passed
@christian-oreilly
Copy link
Collaborator

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
Copy link
Member Author

scott-huberty commented Jul 25, 2024

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:

  • if they have mne >=1.7, but don't have edifo installed, MNE will say "you need to pip/conda install edfio"
  • if they have mne < 1.7, but don't have eflib-python installed, MNE will say "you need to pip/conda install edflib-python"

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
Copy link
Member Author

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

scott-huberty added a commit to scott-huberty/pylossless that referenced this pull request Jul 26, 2024
- This workflow inadverdently got committed in lina-usc#161 i.e. it shoudl never have entered the codebase on main.
scott-huberty added a commit that referenced this pull request Jul 26, 2024
* MAINT: remove warning filters

Now that MNE ICALABEL .7 is out, which has my upstream fix, we should no longer need these futurewarnings as long as the CI's install .7

* FIX: remove stale GHA workflow

- This workflow inadverdently got committed in #161 i.e. it shoudl never have entered the codebase on main.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants