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

ENH: Allow selecting Qt bindings using build selectors #136

Merged
merged 6 commits into from
Jul 8, 2024

Conversation

larsoner
Copy link
Contributor

@larsoner larsoner commented Jul 6, 2024

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

I'm not convinced we should require pyqt in our mne base recipe. It seems like people should nowadays choose between installing pyqt (Qt5) or pyside6 (Qt6) when doing conda install mne. WDYT @hoechenberger ?

@larsoner
Copy link
Contributor Author

larsoner commented Jul 6, 2024

@conda-forge-admin, please rerender

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@hoechenberger
Copy link
Member

hoechenberger commented Jul 6, 2024

@larsoner I think we should depend on pyside6, actually, as soon as the interactive plotting problem has been fixed

in fact i believe this is a change we should do upstream in MNE too

also i wonder if there is something like a python-qt-bindings meta package on conda-forge? for when (if) pyqt6 lands

FWIW mnelab requires pyside6

installing MNE without a qt binding i would actually consider a packaging bug as users would end up with an incomplete install

cc @cbrnr

@larsoner
Copy link
Contributor Author

larsoner commented Jul 6, 2024

@larsoner I think we should depend on pyside6, actually, as soon as the interactive plotting problem has been fixed

Not all packages have gotten rid of their pyqt dependency. For example spyder and matplotlib both still depend on it. So changing our recipe to depend explicitly on pyside6 we'll run into concurrent qt-main + qt6-main installation issues like we're currently hitting in mne-installers (and hit a couple of weeks ago in MNE-Python). So I think someday sure we should change it to pyside6 but I think it's too early.

@hoechenberger
Copy link
Member

In that case I'd suggest we'd better hold off merging this PR
too, because what I'm hearing is that currently, to get a working MNE, users would need to install pyqt(5) anyway. so let's leave it as-is until we can fully migrate to pyside6, wdyt?

@larsoner
Copy link
Contributor Author

larsoner commented Jul 6, 2024

what I'm hearing is that currently, to get a working MNE, users would need to install pyqt(5) anyway.

They could install that or pyside6 (but in that case, don't install Spyder). What this PR does is give users a choice... but also forces them to make a choice.

@hoechenberger
Copy link
Member

Bot you said matplotlib also depends on pyqt(5), so it's not really a choice at this time for many (mist?) users

@larsoner
Copy link
Contributor Author

larsoner commented Jul 6, 2024

Bot you said matplotlib also depends on pyqt(5), so it's not really a choice at this time for many (mist?) users

matplotilb, yes, but matplotlib-base does not. And matplotlib is really just matplotib-base plus tornado and pyqt.

In the meantime I'm pushing a less controversial change to add a mne-noqt that mne now depends on. It's everything but pyqt. That way someone who does conda install mne still gets a usable Qt-enabled env, but someone who wants to use PySide6 can do conda install mne-noqt pyside6 (which is really the thing I'm trying to simplify).

@hoechenberger
Copy link
Member

But would anyone actually want to do that if they then cannot really use matplotlib?

@larsoner
Copy link
Contributor Author

larsoner commented Jul 6, 2024

But would anyone actually want to do that if they then cannot really use matplotlib?

You mean the matplotlib Python library? They can totally use that!

I think we need to be explicit about what's meant by "matplotlib" -- if we're talking about the matplotlib conda recipe, no you wouldn't want to conda install matplotlib because it depends on pyqt. You'd want to install matplotilb-base, which comes with mne-noqt already (and actually mne-base for that matter). So doing

conda install mne-noqt pyside6

a user would have a fully functional conda env with fully functional matplotlib and PySide6 libraries. They would not have the matplotilb conda-forge package installed (just matplotlib-base).

So above when I said:

Not all packages have gotten rid of their pyqt dependency. For example spyder and matplotlib both still depend on it.

I meant not all conda-forge packages / recipes, not the underlying Python libraries themselves. Matplotlib as a Python library works perfectly well with PySide6 and does not depend on PyQt, but their conda-forge recipe/package matplotlib depends on pyqt (whereas matplotilb-base does not). Though I guess in the case of Spyder their Python library/package itself appears to have an explicit PyQt5 dependency actually, so it's more restrictive.

@hoechenberger
Copy link
Member

hoechenberger commented Jul 7, 2024

Ah, thanks, my misunderstanding was that I thought that matplotlib-base wouldn't work with pyside6

do you think that instead of providing separate packages, we could do something like "build variants" with varying deps? basically employing the mechanism that allows users to switch between BLAS implementations, only here we want to give them the choice of different qt bindings

we'd probably need to as conda-forge staff for help

@hoechenberger
Copy link
Member

you are incredible, eric!

@larsoner
Copy link
Contributor Author

larsoner commented Jul 8, 2024

Hah, let's see if it works first!

If it does then we can merge then try using the pyside6 variant in mne-tools/mne-installers#273 🤞

@larsoner larsoner changed the title BUG: Dont explicitly require qt ENH: Allow selecting Qt bindings using build selectors Jul 8, 2024
@larsoner larsoner merged commit 4b9b1a6 into conda-forge:main Jul 8, 2024
6 checks passed
@larsoner larsoner deleted the qt branch July 8, 2024 20:33
Copy link
Member

@hoechenberger hoechenberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💪

@hoechenberger
Copy link
Member

hoechenberger commented Jul 9, 2024

Shall we do something similar with mne-qt-browser? we could then even use the selectors in this recipe here and pick the correct mne-qt-browser variant conditionally such that MNE's and the Qt browser's Qt bindings match

@cbrnr
Copy link

cbrnr commented Jul 9, 2024

Hopefully everything will just work with pyside6 in the future... but great work @larsoner! Just a minor nitpick, I'd prefer the variants to be named consistently, i.e. pyside6 and pyqt6 (or pyside and pyqt), not sure if that's possible/feasible.

@hoechenberger
Copy link
Member

@cbrnr the conda-forge packages are called pyqt (for PyQt5) and pyside6 (for PySide6)

there are no pyqt6 packages yet

@cbrnr
Copy link

cbrnr commented Jul 9, 2024

Ah OK, I had a feeling that it's the conda packages that are inconsistent (pyqt should really be called pyqt5). So all good then, and if they ever add a PyQt6 package, maybe they'll name it pyqt6 anyway.

@hoechenberger
Copy link
Member

yep I think this is what will happen! 🤞

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.

3 participants