Skip to content

Update pyproject.toml adding pyqt5 binding#199

Closed
timonmerk wants to merge 1 commit intomne-tools:mainfrom
timonmerk:patch-1
Closed

Update pyproject.toml adding pyqt5 binding#199
timonmerk wants to merge 1 commit intomne-tools:mainfrom
timonmerk:patch-1

Conversation

@timonmerk
Copy link
Contributor

When installing mne-qt-browser under Windows 11, mne version 1.5.1, pip install mne-qt-browser does not install the required qt binding.

raw.plot() therefore results in the error:

No Qt bindings could be found
Traceback (most recent call last):
File "", line 1, in
File "C:\Users\ICN_admin\Anaconda3\envs\bispectra\lib\site-packages\mne\io\base.py", line 1825, in plot
return plot_raw(
File "", line 12, in plot_raw
File "C:\Users\ICN_admin\Anaconda3\envs\bispectra\lib\site-packages\mne\viz\raw.py", line 400, in plot_raw
fig = _get_browser(show=show, block=block, **params)
File "C:\Users\ICN_admin\Anaconda3\envs\bispectra\lib\site-packages\mne\viz_figure.py", line 698, in _get_browser
fig = backend._init_browser(**kwargs)
AttributeError: 'NoneType' object has no attribute '_init_browser'

Installing pyqt5 with pip installl pyqt5 fixes the issue as described here. Therefore I added the dependency to the pyproject.toml

@larsoner
Copy link
Member

I don't think we want to do this. In principle mne-qt-browser can use PyQt5, PyQt5, PySide2, or PySide6, hence why we don't force users to install any specific one.

@mscheltienne
Copy link
Member

The error message raised when importing the browser should explicitly tell the user that he needs to choose and install one of the bindings, PyQt5, PyQt6, PySide2, or Pyside6. Is that not the case?

@larsoner
Copy link
Member

... and if it's not, I consider it a qtpy bug and we should open an issue there since we really rely on them to do all the binding business

@cbrnr
Copy link
Contributor

cbrnr commented Sep 21, 2023

I don't think the message mentions specific bindings. I've wanted to add this at some point in spyder-ide/qtpy#221, but it seems like it wasn't important enough to pursue it further.

@hoechenberger
Copy link
Member

hoechenberger commented Sep 21, 2023

The error message raised when importing the browser should explicitly tell the user that he needs to choose and install one of the bindings, PyQt5, PyQt6, PySide2, or Pyside6.

I agree we should produce a helpful error message

Or simply explicitly depend on one of those bindings, as done in this PR

It's not cool you end up with a defunct package after pip installing.

@mscheltienne
Copy link
Member

How about a post-install script which checks for the presence of at least one binding, and else install a default one? That has to be doable with setuptools?

@cbrnr
Copy link
Contributor

cbrnr commented Sep 22, 2023

TBH, I would try to improve the error message in QtPy. Installing something with a post-install script could lead to problems (I'm not an expert, but many package dependency analysis tools will probably not be able to parse such custom post-install things).

Another alternative would be to ditch QtPy and use a specific binding as @hoechenberger suggested. It looks like PyQt6 would be the best option, but I'm sure this will trigger other issues (most likely related to PyVista).

@cbrnr
Copy link
Contributor

cbrnr commented Sep 22, 2023

A third option could be that our backend issues an appropriate error message (i.e. we catch the generic one from QtPy and turn it into something more useful).

@larsoner
Copy link
Member

larsoner commented Sep 29, 2023

Agreed QtPy would be the right place but who knows if they'll accept a fix based on spyder-ide/qtpy#221 going stale. I'll close this PR and @timonmerk if you're up for it feel free to open another PR to improve our first qtpy import with a try/except. The annoying part is that they raise PythonQtError, which is in their namespace... which you can only get if you can import qtpy. So you'll probably need something like:

try:
    from qtpy...
except Exception as exc:
    if exc.__class__.__name__ == "PythonQtError":
         raise ImportError(...) from None
    else:
         raise

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.

5 participants