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

Include MNELAB #163

Closed
wants to merge 21 commits into from
Closed

Include MNELAB #163

wants to merge 21 commits into from

Conversation

hoechenberger
Copy link
Member

Closes #82

Just want to see if it works

@hoechenberger
Copy link
Member Author

The ARM build is expected to fail, as we're still waiting for the migration to complete on conda-forge

@larsoner
Copy link
Member

All green except ARM as expected, great!

@hoechenberger
Copy link
Member Author

If you have time, could you download and test the artifacts?

Also I think we need some sort of CI test for MNELAB

Mind you, I believe we now have Qt5 and Qt6 installed in parallel! Who knows what kind d of interactions might occur 😱

@larsoner
Copy link
Member

We should just switch to Qt6, the rest of MNE should be compatible with it already

I'll test on my M1 once it's working there :)

@larsoner
Copy link
Member

... actually I'm not sure if mayavi is compatible. We'd need to check for mne-kit-gui :(

@hoechenberger
Copy link
Member Author

I don't think spyder supports
Qt6 either

@hoechenberger
Copy link
Member Author

I remember the conda-forge folks spending quite some time to ensure isolation between Qt5 and Qt6, so at least in theory things should work. But who knows how well downstream packages can handle the simultaneous presence of both

@hoechenberger
Copy link
Member Author

The ARM build is now on conda-forge, restarting CI.

@hoechenberger
Copy link
Member Author

Ouch, dependency resolution error 😳 this is going to be a pain to pin down 😑

@larsoner
Copy link
Member

Locally on M1 when I try to make an env with our package list I get:

$ mamba env create -n test -f my-env.yml 
...
Encountered problems while solving:
  - package mnelab-0.8.5-py38h10201cd_0 requires python >=3.8,<3.9.0a0, but none of the providers can be installed

which is weird because the builds for other Python versions do show up in https://anaconda.org/conda-forge/mnelab/files . Maybe CDNs are just way behind or something?

@larsoner
Copy link
Member

Looks like it's actually a conflict with vtk =9.2.2. With even just:

name: test
channels:
  - conda-forge
dependencies:
  - mnelab =0.8.5
  - python =3.10.8
  - vtk =9.2.2

I get:

...
UnsatisfiableError: The following specifications were found to be incompatible with each other:

Output in format: Requested package -> Available versions

Package python conflicts for:
vtk=9.2.2 -> loguru -> python[version='3.10.*|3.11.*|>=3.5|>=3.6|3.8.*|3.9.*']
mnelab=0.8.5 -> python[version='>=3.10,<3.11.0a0|>=3.10,<3.11.0a0|>=3.8,<3.9.0a0|>=3.8,<3.9.0a0|>=3.9,<3.10.0a0',build=*_cpython]
vtk=9.2.2 -> python[version='>=3.10,<3.11.0a0|>=3.10,<3.11.0a0|>=3.11,<3.12.0a0|>=3.8,<3.9.0a0|>=3.8,<3.9.0a0|>=3.9,<3.10.0a0|>=3.9,<3.10.0a0',build=*_cpython]
python=3.10.8
mnelab=0.8.5 -> matplotlib-base[version='>=3.6.2'] -> python[version='3.10.*|3.8.*|3.9.*|>=3.11,<3.12.0a0|>=3.7|>=3.6|>=3.5',build=*_cpython]

Package expat conflicts for:
vtk=9.2.2 -> expat[version='>=2.4.9,<3.0a0|>=2.5.0,<3.0a0']
vtk=9.2.2 -> python[version='>=3.9,<3.10.0a0'] -> expat[version='>=2.4.1,<3.0a0']

Package setuptools conflicts for:
python=3.10.8 -> pip -> setuptools
mnelab=0.8.5 -> mne-base[version='>=1.0.0'] -> setuptools

Package freetype conflicts for:
vtk=9.2.2 -> ffmpeg[version='>=4.4.2,<5.0a0'] -> freetype[version='>=2.10.4,<3.0a0']
vtk=9.2.2 -> freetype[version='>=2.12.1,<3.0a0']

Package libcblas conflicts for:
mnelab=0.8.5 -> numpy[version='>=1.20.0'] -> libcblas[version='>=3.9.0,<4.0a0']
vtk=9.2.2 -> numpy -> libcblas[version='>=3.9.0,<4.0a0']

Rolling back to vtk 9.1 doesn't help because then there are conflicts with openmeeg :(

@cbrnr
Copy link
Collaborator

cbrnr commented Dec 14, 2022

Is there anything MNELAB can do to help solve this conflict? I'm not pinning any dependencies (only >=) because MNELAB should work always work with the latest versions. Python versions 3.8–3.11 should be supported (I'm aiming for the last three versions, right now these are even four).

@hoechenberger
Copy link
Member Author

@larsoner I suspect it has to do with me pinning matplotlib to the latest version... But 3.6.2 is required to work around bugs in PySide 6.4 😩

cbrnr/mnelab#362

@hoechenberger
Copy link
Member Author

hoechenberger commented Dec 14, 2022

@larsoner I suspect it has to do with me pinning matplotlib to the latest version... But 3.6.2 is required to work around bugs in PySide 6.4 😩

cbrnr/mnelab#362

This assumption was incorrect. I can reproduce the issue by simply requesting vtk and pyside6.

❯ mamba create -n test pyside6=6.4.1 vtk=9.2                                                                     ─╯
[...]
Encountered problems while solving:
  - package pyside6-6.4.1-py310h6bbb1e2_0 requires libclang >=13.0.1,<14.0a0, but none of the providers can be installed

We should get in touch with the VTK feedstock maintainers.

@hoechenberger
Copy link
Member Author

I reported it to the VTK and PySide6 feedstock maintainers:

conda-forge/vtk-feedstock#269
conda-forge/pyside2-feedstock#159

@larsoner
Copy link
Member

Great @hoechenberger ! Now we wait ...

@hoechenberger
Copy link
Member Author

@larsoner Could be a long time…

conda-forge/pyside2-feedstock#159 (comment)

@hoechenberger
Copy link
Member Author

I think we should simply build the M1 installers without MNELAB, as the ETA of the OpenSSL migration, which seems to be required to resolve the issue, is unclear (and probably a few months away)

@hoechenberger hoechenberger changed the title Include MNELAB Include MNELAB (except for Apple M1) Dec 14, 2022
@larsoner
Copy link
Member

Works for me, feel free to take out of draft mode and mark for merge-when-green @hoechenberger

@hoechenberger hoechenberger marked this pull request as ready for review December 14, 2022 12:28
@hoechenberger
Copy link
Member Author

@larsoner Are we sure the Windows installer problems @mscheltienne observed recently are gone now, too? I cannot test, unfortunately

@larsoner
Copy link
Member

I tested locally on Windows last week and things were fine

@hoechenberger hoechenberger enabled auto-merge (squash) December 14, 2022 12:30
@hoechenberger
Copy link
Member Author

Great! Enabling automerge.

@cbrnr
Copy link
Collaborator

cbrnr commented Apr 23, 2023

I don't remember exactly our discussion, but MNELAB should work fine even without this patch. Installing both PySide6 and PyQt6 in parallel should also work.

@hoechenberger
Copy link
Member Author

No, we had trouble installing both in parallel, unfortunately. And Matplotlib doesn't work with PySide6 without the patch adding the input hook

@cbrnr
Copy link
Collaborator

cbrnr commented Apr 23, 2023

Do you remember what exactly the problem was? I always have both packages installed and everything works.

@hoechenberger
Copy link
Member Author

I always have both packages installed and everything works.

Yes but you don't work with conda-forge packages, do you?

@cbrnr
Copy link
Collaborator

cbrnr commented Apr 23, 2023

No. But I can try if you want.

@cbrnr
Copy link
Collaborator

cbrnr commented Apr 24, 2023

@hoechenberger I can install both pyside6 and pyqt in parallel with no errors. The only thing that happens is that pyside6 gets downgraded from 6.5.0 to 6.4.3, not sure why. It also seems like pyqt still installs PyQt5 (5.15.7), there is no version 6.

@hoechenberger
Copy link
Member Author

hoechenberger commented Apr 24, 2023

Parallel installation does work fine, but we had trouble actually using it then. There was confusion regarding the shared libraries in some situations. Just installing is not enough of a test

There is no PyQt6 on conda-forge, only PySide6

@cbrnr
Copy link
Collaborator

cbrnr commented Apr 24, 2023

I ran MNELAB and imported both PySide6 and PyQt5. If you can point me to the issue I could try if the problem still exists.

@hoechenberger
Copy link
Member Author

There's a test case here in this branch in build.yml

@cbrnr
Copy link
Collaborator

cbrnr commented Apr 24, 2023

OK, yes, it seems like pyside6 doesn't work in the presence of pyqt. I've submitted a bug report: conda-forge/pyside2-feedstock#183

@hoechenberger
Copy link
Member Author

Thanks @cbrnr!

@larsoner
Copy link
Member

Any chance to transition to qtpy in MNELAB?

@cbrnr
Copy link
Collaborator

cbrnr commented May 11, 2023

Any chance to transition to qtpy in MNELAB?

I've used qtpy previously, but switched away for three reasons:

  1. I did not want to support Qt versions < 6 anymore (there are some weird bugs like font rendering that are not going to be fixed).

  2. PySide6 has some nice features that PyQt6 doesn't offer, like more Pythonic names.

  3. PyQt6 is licensed under the GPL, whereas PySide6 uses the LGPL. I'm not an expert, but the LGPL seems like the much better option here, because it is clear that we can just use it without having to worry about the license of MNELAB. For GPL, although BSD is compatible, I'm not entirely sure that we can use it without having to change our own license to GPL.

  4. We still wanted to include Qt bindings as a requirement, so people don't have to figure out why MNELAB does not work after pip install mnelab. Just including e.g. PySide6 defeats the purpose of qtpy, and I did not want a brittle custom solution in setup.py (which BTW wouldn't even be possible now that we switched to pyproject.toml). I did not want users to install with pip install mnelab[pyside6], because I think almost no one knows how this works (I know it would be documented, but few people read the docs and would just use what they know from other packages, which is pip install mnelab).

In summary, I'm open to switching back, but these issues need to be addressed. The second one is not a real issue, because AFAIR I'm not even using PySide6-specific features.

This article contains all the details: https://www.pythonguis.com/faq/pyqt6-vs-pyside6/

They mention a custom solution to support both PyQt6 and PySide6, maybe that would be an option. But I still don't know how to handle requirements even in this case.

@hoechenberger
Copy link
Member Author

This should work now:

conda-forge/qt-main-feedstock#167

@hoechenberger
Copy link
Member Author

My understanding is that now, thanks to conda-forge/qt-main-feedstock#167, parallel installation of Qt5 and Qt6 should work.

Since we still don't have PyQt6 on conda-forge, we have to rely on PySide6. But the following issue / PR is stalled:
https://codereview.qt-project.org/c/pyside/pyside-setup/+/454254

@cbrnr Feel like bumping this again?

@cbrnr
Copy link
Collaborator

cbrnr commented Aug 24, 2023

Yes. Here's the corresponding issue: https://bugreports.qt.io/browse/PYSIDE-2192

@cbrnr
Copy link
Collaborator

cbrnr commented Aug 24, 2023

@hoechenberger maybe you (and others) could vote for this issue to show that it is important?

@hoechenberger
Copy link
Member Author

hoechenberger commented Aug 24, 2023

@cbrnr I just voted for this issue

@cbrnr
Copy link
Collaborator

cbrnr commented Aug 24, 2023

Thanks! But unfortunately, the answer is not promising. I don't even see the error on Windows. I mean, I probably wouldn't be able to help anyway, but do we know someone who could?

@hoechenberger
Copy link
Member Author

I'm really surprised this issue isn't high-prio for them. Are we potentially missing something obvious here? Shouldn't there be literally masses of users running into this problem?

@cbrnr
Copy link
Collaborator

cbrnr commented Aug 24, 2023

Hard to say. On macOS, people get to use the macosx backend by default, which works great. Not sure about Windows and Linux, but maybe people just use PyQt? The problem is specific to conda-forge and Qt 6, which is only available as PySide6 there. If you use pip, then you also have PyQt6. If you are happy with Qt 5, PyQt5 is also available on conda-forge. So yes, maybe really a niche problem.

@hoechenberger
Copy link
Member Author

@cbrnr Any changes here..?

@cbrnr
Copy link
Collaborator

cbrnr commented Dec 19, 2023

Not that I know of. My list of caveats still applies, if you mean switching to qtpy.

@hoechenberger
Copy link
Member Author

I meant the problem that Matplotlib had with PySide6. It seems the issue inside PySide6 still hasn't been addressed; but I was hoping that maybe MPL devs found some workaround.

I cannot believe this is just going unaddressed for so long??? Is nobody using that? I don't get it?

@hoechenberger hoechenberger deleted the mnelab branch February 27, 2024 16:50
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.

Add MNELAB (requires Qt 6)
4 participants