Skip to content

MAINT: Migrate from PyQt5 to PySide6#510

Merged
rob-luke merged 6 commits intomne-tools:mainfrom
florin-pop:move_to_pyside6
Apr 28, 2023
Merged

MAINT: Migrate from PyQt5 to PySide6#510
rob-luke merged 6 commits intomne-tools:mainfrom
florin-pop:move_to_pyside6

Conversation

@florin-pop
Copy link
Copy Markdown
Collaborator

@florin-pop florin-pop commented Apr 27, 2023

Fixes #471 by:

  • replacing PyQt5 with pyside6 in requirements.txt which is also used by Circle CI / Azure pipelines
  • replacing PyQt6 with pyside6 for github workflows
  • adding a mitigation for ensuring that some tests are not failing due to a QT deprecation warning

Contributed with ❤️ by AE Studio

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 27, 2023

Codecov Report

Merging #510 (13d4819) into main (2f7ceb8) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #510      +/-   ##
==========================================
+ Coverage   95.36%   95.37%   +0.01%     
==========================================
  Files          69       69              
  Lines        2781     2791      +10     
  Branches      397      400       +3     
==========================================
+ Hits         2652     2662      +10     
  Misses         65       65              
  Partials       64       64              
Impacted Files Coverage Δ
mne_nirs/tests/test_examples.py 81.25% <100.00%> (+1.25%) ⬆️
mne_nirs/visualisation/tests/test_visualisation.py 100.00% <100.00%> (ø)

def run_script_and_check(test_file_path):
with open(test_file_path) as fid:
with open(test_file_path) as fid, warnings.catch_warnings():
# Ignore deprecation warning caused by
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I added this workaround in order to attempt to fix these kind of failures which are caused by a warning that originates from https://github.com/mne-tools/mne-python/blob/cecbf0fb1dc919254bb6095db491b2d7c5f68003/mne/viz/backends/_utils.py#L164
Do you have a better suggestion on how to mitigate this? Maybe we could submit a patch to upstream?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes a fix in MNE-Python would be appreciated!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What should a fix look like for this? I guess we can't remove the code because MNE-Python still needs to support previous versions of Qt? And it feels dangerous to catch and ignore the errors in MNE-Python. Any suggestions @larsoner ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In MNE we should check the Qt version and set this attribute conditionally. Or do whatever the Qt deprecation says we should do for new enough versions (set a different attribute, no need to set anything, etc. -- I haven't read what they say to do yet)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

... and here you could conditionally catch warnings or not depending on the MNE and/or At version if you want

@florin-pop florin-pop changed the title MAINT: Prefer PySide6 in testing MAINT: Migrate from PyQt5 to PySide6 Apr 28, 2023
@florin-pop florin-pop marked this pull request as ready for review April 28, 2023 05:18
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.

Follow MNE-Python and remove pyqt5 and move to pyside6

3 participants