Use PySide6 #9586

JHolba · 2024-12-18T12:20:34Z

Upgrade from qtpy with pyqt5 to PySide6.
Using PySide6 directly should give us better typing.

PR title captures the intent of the changes, and is fitting for release notes.
Added appropriate release note label
Commit history is consistent and clean, in line with the contribution guidelines.
Make sure unit tests pass locally after every commit (git rebase -i main --exec 'pytest tests/ert/unit_tests -n logical -m "not integration_test"')

When applicable

When there are user facing changes: Updated documentation
New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
Large PR: Prepare changes in small commits for more convenient review
Bug fix: Add regression test for the bug
Bug fix: Create Backport PR to latest release

codspeed-hq · 2024-12-18T13:45:49Z

CodSpeed Performance Report

Merging #9586 will not alter performance

_{Comparing JHolba:use-pyside6 (fcb1ddc) with main (d9088eb)}

Summary

✅ 22 untouched benchmarks

jonathan-eq · 2024-12-19T07:07:06Z

It fails locally on MacOS with python3.12, but I got it working by modifying:

@pytest.fixture
def panel_with_localization_on(qtbot: QtBot):
    def func(settings, ensemble_size):
        widget = AnalysisModuleVariablesPanel(settings, ensemble_size)
        qtbot.addWidget(widget)
        widget.show()
        check_box = widget.findChild(QCheckBox, name="localization")
        qtbot.mouseClick(check_box, Qt.MouseButton.LeftButton)
        return settings, widget

    yield func

jonathan-eq · 2024-12-19T07:07:37Z

When it failed it emitted:

---------------------------------------------------------------------------------------------------------------------------------------- Captured Qt messages ----------------------------------------------------------------------------------------------------------------------------------------
QtWarningMsg: Populating font family aliases took 77 ms. Replace uses of missing font family "Sans Serif" with one that exists to avoid this cost. 
QtWarningMsg: This plugin does not support propagateSizeHints()

codecov-commenter · 2024-12-19T07:53:36Z

Codecov Report

Attention: Patch coverage is 87.39837% with 62 lines in your changes missing coverage. Please review.

Project coverage is 91.84%. Comparing base (e5c7953) to head (fcb1ddc).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
.../tools/plot/customize/limits_customization_view.py	30.00%	14 Missing ⚠️
src/ert/gui/tools/plugins/process_job_dialog.py	61.11%	7 Missing ⚠️
.../ert/gui/tools/plot/widgets/clearable_line_edit.py	62.50%	6 Missing ⚠️
src/ert/gui/ertwidgets/searchbox.py	60.00%	4 Missing ⚠️
src/ert/gui/simulation/view/realization.py	82.60%	4 Missing ⚠️
...t/gui/tools/plot/plot_ensemble_selection_widget.py	60.00%	4 Missing ⚠️
src/ert/gui/ertwidgets/listeditbox.py	72.72%	3 Missing ⚠️
src/ert/gui/simulation/run_dialog.py	86.95%	3 Missing ⚠️
src/ert/gui/model/snapshot.py	90.00%	2 Missing ⚠️
src/ert/gui/simulation/view/update.py	77.77%	2 Missing ⚠️
... and 10 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9586      +/-   ##
==========================================
- Coverage   91.86%   91.84%   -0.02%     
==========================================
  Files         433      433              
  Lines       26775    26798      +23     
==========================================
+ Hits        24596    24613      +17     
- Misses       2179     2185       +6

Flag	Coverage Δ
cli-tests	`39.72% <0.20%> (-0.03%)`	⬇️
everest-models-test	`34.54% <0.20%> (-0.03%)`	⬇️
gui-tests	`72.11% <81.30%> (-0.01%)`	⬇️
integration-test	`37.17% <0.20%> (-0.02%)`	⬇️
performance-tests	`51.92% <61.17%> (-0.03%)`	⬇️
test	`40.66% <0.20%> (+0.23%)`	⬆️
unit-tests	`74.18% <75.20%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JHolba · 2024-12-19T07:54:24Z

It fails locally on MacOS with python3.12, but I got it working by modifying:

@pytest.fixture
def panel_with_localization_on(qtbot: QtBot):
    def func(settings, ensemble_size):
        widget = AnalysisModuleVariablesPanel(settings, ensemble_size)
        qtbot.addWidget(widget)
        widget.show()
        check_box = widget.findChild(QCheckBox, name="localization")
        qtbot.mouseClick(check_box, Qt.MouseButton.LeftButton)
        return settings, widget

    yield func

Thank you for the help :)

Upgrade from qtpy with pyqt5 to PySide6. Using PySide6 directly should give us better typing.

jonathan-eq · 2024-12-20T11:20:51Z

src/ert/gui/ertwidgets/searchbox.py

+    def keyPressEvent(self, arg__1: QKeyEvent) -> None:
+        if arg__1 is not None and arg__1.key() == Qt.Key.Key_Escape:


Why are we checking if it is not None, if the parameter cannot be None?

jonathan-eq · 2024-12-20T11:29:16Z

src/ert/gui/model/real_list.py

-    def parent(self, child: QModelIndex | None = None) -> QObject | None:
+    def parent(
+        self, child: QModelIndex | QPersistentModelIndex | None = None
+    ) -> QObject | QModelIndex:


Do we need QObject here?

we have no choice. we are overriding existing methods.
https://doc.qt.io/qtforpython-6/PySide6/QtCore/QObject.html#PySide6.QtCore.QObject.parent
https://doc.qt.io/qtforpython-6/PySide6/QtCore/QAbstractItemModel.html#PySide6.QtCore.QAbstractItemModel.parent

jonathan-eq · 2024-12-20T11:31:08Z

src/ert/gui/model/snapshot.py

        if parent is None:
            parent = QModelIndex()
        parent_item = self.root if not parent.isValid() else parent.internalPointer()

        if parent.column() > 0:
            return 0

-        return len(parent_item.children)
+        return len(parent_item.children)  # type: ignore


Why does this need type: ignore?

jonathan-eq · 2024-12-20T11:32:41Z

src/ert/gui/model/snapshot.py

-    def parent(self, child: QModelIndex | None = None) -> QObject | None:
+    def parent(
+        self, child: QModelIndex | QPersistentModelIndex | None = None
+    ) -> QObject | QModelIndex:


Does this also need QObject?
For QModelIndex, is it possible to have the typing include specifically what the index is pointing to?

both of the parent overloads come from inherited methods. the override must handle them both

jonathan-eq · 2024-12-20T11:32:57Z

src/ert/gui/model/snapshot.py

        if child is None or not child.isValid():
            return QModelIndex()

-        parent_item = child.internalPointer().parent
+        parent_item = child.internalPointer().parent  # type:ignore


Another type:ignore. Maybe we dont need it?

jonathan-eq · 2024-12-20T11:33:09Z

src/ert/gui/model/snapshot.py

+        self,
+        index: QModelIndex | QPersistentModelIndex,
+        role: int = Qt.ItemDataRole.DisplayRole,
+    ) -> Any:


Should we use Any?

It is an override. It comes from QAbstractItemModel

jonathan-eq · 2024-12-20T11:33:43Z

src/ert/gui/model/snapshot.py

-    def _real_data(_: QModelIndex, node: RealNode, role: int) -> Any:
+    def _real_data(
+        _: QModelIndex | QPersistentModelIndex, node: RealNode, role: int
+    ) -> Any:


Another Any

it is only used by the data() method which returns any

jonathan-eq · 2024-12-20T11:34:31Z

src/ert/gui/model/snapshot.py

@@ -445,7 +464,7 @@ def _fm_step_data(
        if role == FileRole:
            data_name = FM_STEP_COLUMNS[index.column()]
            if data_name in {ids.STDOUT, ids.STDERR}:
-                return node.data.get(data_name, QVariant())
+                return node.data.get(data_name, None)


Is None redundant here?

ids.STDOUT and ids.STDERR are both optional in the typeddict. we need the None if they are not defined

jonathan-eq · 2024-12-20T11:50:22Z

src/ert/gui/simulation/run_dialog.py

@@ -307,17 +306,18 @@ def on_snapshot_new_iteration(
    ) -> None:
        if not parent.isValid():
            index = self._snapshot_model.index(start, 0, parent)
+            iteration = cast(Node, index.internalPointer()).id_


Should we cast it to Node or IterationNode? I think the point of having Node prefixed with underscore was that it would not be imported elsewhere

jonathan-eq · 2024-12-20T11:54:01Z

src/ert/gui/simulation/view/progress_widget.py

 )

 from ert.ensemble_evaluator.state import ENSEMBLE_STATE_FAILED, REAL_STATE_TO_COLOR


 class ProgressWidget(QFrame):
    def __init__(self) -> None:
-        QWidget.__init__(self)
+        QFrame.__init__(self)


Should we change this to super().__init__()?

jonathan-eq · 2024-12-20T11:54:44Z

src/ert/gui/simulation/view/progress_widget.py

@@ -110,5 +109,5 @@ def update_progress(self, status: dict[str, int], realization_count: int) -> Non
            self.stop_waiting_progress_bar()
        self.repaint_components()

-    def resizeEvent(self, a0: Any, event: Any = None) -> None:
+    def resizeEvent(self, event: Any = None) -> None:


Is there a more specific event type we can use?

jonathan-eq · 2024-12-20T11:57:09Z

src/ert/gui/simulation/view/update.py

@@ -127,7 +127,7 @@ def iteration(self) -> int:
    def _insert_status_message(self, message: str) -> None:
        item = QListWidgetItem()
        item.setText(message)
-        item.setFlags(item.flags() & ~Qt.ItemFlags(Qt.ItemFlag.ItemIsEnabled))
+        item.setFlags(item.flags() & ~Qt.ItemFlag.ItemIsEnabled)


Can we use item.setFlag instead?

there is no setFlag method on QListWidgetItem

jonathan-eq · 2024-12-20T11:58:02Z

src/ert/gui/suggestor/suggestor.py

@@ -131,6 +134,7 @@ def __init__(
        self.setStyleSheet(f"background-color: {LIGHT_GREY}; color: black")
        self.__layout.setContentsMargins(32, 47, 32, 16)
        self.__layout.setSpacing(32)
+        self.notifier = notifier


Do we need access to the notifier?

jonathan-eq

Very good! I have some questions, see comments :)

JHolba self-assigned this Dec 18, 2024

JHolba marked this pull request as draft December 18, 2024 12:20

JHolba added the release-notes:maintenance Automatically categorise as maintenance change in release notes label Dec 18, 2024

JHolba force-pushed the use-pyside6 branch 10 times, most recently from be4a6dc to c7090c4 Compare December 18, 2024 13:24

JHolba force-pushed the use-pyside6 branch 2 times, most recently from 429fc1e to a397c37 Compare December 18, 2024 15:01

JHolba force-pushed the use-pyside6 branch from f973a74 to 0ff1439 Compare December 19, 2024 07:38

JHolba force-pushed the use-pyside6 branch from 0ff1439 to 1f309aa Compare December 19, 2024 07:55

Use PySide6

8f67e6a

Upgrade from qtpy with pyqt5 to PySide6. Using PySide6 directly should give us better typing.

JHolba force-pushed the use-pyside6 branch from e6dbe25 to 09e02d1 Compare December 19, 2024 12:24

Add additional typing to qt components

fcb1ddc

JHolba force-pushed the use-pyside6 branch from 09e02d1 to fcb1ddc Compare December 19, 2024 13:56

JHolba marked this pull request as ready for review December 19, 2024 14:10

jonathan-eq self-requested a review December 20, 2024 06:57

jonathan-eq reviewed Dec 20, 2024

View reviewed changes

jonathan-eq requested changes Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use PySide6 #9586

Use PySide6 #9586

JHolba commented Dec 18, 2024 •

edited

Loading

codspeed-hq bot commented Dec 18, 2024 •

edited

Loading

jonathan-eq commented Dec 19, 2024

jonathan-eq commented Dec 19, 2024

codecov-commenter commented Dec 19, 2024 •

edited

Loading

JHolba commented Dec 19, 2024

jonathan-eq Dec 20, 2024

jonathan-eq Dec 20, 2024

JHolba Dec 23, 2024

jonathan-eq Dec 20, 2024

jonathan-eq Dec 20, 2024

JHolba Dec 23, 2024

jonathan-eq Dec 20, 2024

jonathan-eq Dec 20, 2024

JHolba Dec 23, 2024

jonathan-eq Dec 20, 2024

JHolba Dec 23, 2024

jonathan-eq Dec 20, 2024

JHolba Dec 23, 2024

jonathan-eq Dec 20, 2024

jonathan-eq Dec 20, 2024 •

edited

Loading

jonathan-eq Dec 20, 2024

jonathan-eq Dec 20, 2024

JHolba Dec 23, 2024

jonathan-eq Dec 20, 2024

jonathan-eq left a comment

		def keyPressEvent(self, arg__1: QKeyEvent) -> None:
		if arg__1 is not None and arg__1.key() == Qt.Key.Key_Escape:

Use PySide6 #9586

Are you sure you want to change the base?

Use PySide6 #9586

Conversation

JHolba commented Dec 18, 2024 • edited Loading

When applicable

codspeed-hq bot commented Dec 18, 2024 • edited Loading

Merging #9586 will not alter performance

Summary

jonathan-eq commented Dec 19, 2024

jonathan-eq commented Dec 19, 2024

codecov-commenter commented Dec 19, 2024 • edited Loading

Codecov Report

JHolba commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathan-eq Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathan-eq left a comment

Choose a reason for hiding this comment

JHolba commented Dec 18, 2024 •

edited

Loading

codspeed-hq bot commented Dec 18, 2024 •

edited

Loading

codecov-commenter commented Dec 19, 2024 •

edited

Loading

jonathan-eq Dec 20, 2024 •

edited

Loading