mne-tools / mne-installers

Installers for MNE-Python.
BSD 3-Clause "New" or "Revised" License
9 stars 8 forks source link

Include MNELAB #163

Closed hoechenberger closed 8 months ago

hoechenberger commented 1 year ago

Closes #82

Just want to see if it works

hoechenberger commented 1 year ago

@larsoner I believe we need to wait until https://github.com/conda-forge/pyside2-feedstock/pull/165 has been closed,otherwise ARM builds will continue to fail

Edit: Or do you think https://github.com/conda-forge/qt-main-feedstock/pull/93 suffices? We need to ensure we're using the latest builds in our CI runs here, maybe we should pin the build numbers

larsoner commented 1 year ago

Qt5/Qt6 simultaneous install problems remain. Might have to wait until we can get PySide6 only. Seems safest anyway

hoechenberger commented 1 year ago

Now with https://github.com/conda-forge/pyside2-feedstock/issues/164 resolved, MNELAB works for me on my M1 Mac:

❯ mamba create -n mnelab mnelab "pyside6=*=*_3"
❯ mamba run -n mnelab mnelab
Screenshot 2023-01-10 at 14 52 40
cbrnr commented 1 year ago

This is great! Finally! Thanks for your persistence everyone!

hoechenberger commented 1 year ago

Don't celebrate too early though, we still need to address some issues before we can ship MNELAB with the installers. But we're one giant step closer now!!

larsoner commented 1 year ago

@hoechenberger any idea if they plan to build PyQt6? For me PySide6 is unusable in Linux because of this, at least when using pip (i.e., plt.ion() does not work because PySide6 does not set up the PyOS_InputHook like PyQt6 does)

cbrnr commented 1 year ago

Using IPython is not an option for you? If not, do you think a feature request would be useful?

larsoner commented 1 year ago

I don't think we can/should force people to use IPython instead of plain Python. But yes a feature request would be useful, feel free to open one with the PySide6 folks, to me it's a major (deal-breaking) deficiency compared to PyQt6 and I imagine a lot of other people will be in the same boat

hoechenberger commented 1 year ago

@hoechenberger any idea if they plan to build PyQt6?

No plans that I know of. You could ask on the PySide feedstock maybe? But I doubt anyone will be willing to put in the effort tbh...

cbrnr commented 1 year ago

It will probably be easier to ask the PySide devs to consider adding an input hook.

hoechenberger commented 1 year ago

@cbrnr I kind of lost track. I believe you filed a bug report upstream, right? Any updates on this?

cbrnr commented 1 year ago

Yes, here: https://bugreports.qt.io/browse/PYSIDE-2192

It seems like they are working on it? https://codereview.qt-project.org/c/pyside/pyside-setup/+/454254

hoechenberger commented 1 year ago

Yes, here: https://bugreports.qt.io/browse/PYSIDE-2192

It seems like they are working on it? https://codereview.qt-project.org/c/pyside/pyside-setup/+/454254

Seems it's stalled. Maybe we should ping them...

cbrnr commented 1 year ago

Seems it's stalled. Maybe we should ping them...

Yeah, not sure, in general I don't like "hey, any update on this, when will this be ready" kind of pings.

hoechenberger commented 1 year ago

My impression was that the implementation is already there and it's just pending review, so maybe folks just forgot about it? But I see what you mean

cbrnr commented 1 year ago

If it's really like that I can ask if there is something I could do to help (e.g. test). I think that's an OK ping.

hoechenberger commented 1 year ago

Still waiting for https://codereview.qt-project.org/c/pyside/pyside-setup/+/454254, it's stalled

cbrnr commented 1 year ago

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 commented 1 year ago

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

cbrnr commented 1 year ago

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

hoechenberger commented 1 year ago

I always have both packages installed and everything works.

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

cbrnr commented 1 year ago

No. But I can try if you want.

cbrnr commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

cbrnr commented 1 year ago

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

hoechenberger commented 1 year ago

Thanks @cbrnr!

larsoner commented 1 year ago

Any chance to transition to qtpy in MNELAB?

cbrnr commented 1 year ago

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 commented 1 year ago

This should work now:

https://github.com/conda-forge/qt-main-feedstock/pull/167

hoechenberger commented 1 year ago

My understanding is that now, thanks to https://github.com/conda-forge/qt-main-feedstock/pull/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 commented 1 year ago

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

cbrnr commented 1 year ago

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

hoechenberger commented 1 year ago

@cbrnr I just voted for this issue

cbrnr commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 11 months ago

@cbrnr Any changes here..?

cbrnr commented 11 months ago

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

hoechenberger commented 11 months ago

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?