Improve handling of missing responses.json #9589

jonathan-eq · 2024-12-18T13:40:03Z

Issue
Resolves #9493

Approach
The commit in this PR:

Makes storage reload and re-validate when changing between ert modes (manage experiment, run experiment, plot experiment)
Disables ensembles with invalid experiments from ensemble_selectors (error is shown as tooltip on hover)
Filters out invalid experiments from dark storage, so plotter won't attempt to plot it.
Reloads storage and re-validates on end of experiment, so ert wont crash if responses.json is deleted mid-run.

(Screenshot of new behavior in GUI if applicable)

(This is when the responses.json file is deleted mid run. Prior to this PR, ert would crash with ValueError: responses.json does not exist)

(This is when responses.json is deleted, and we open the plotter. The ensemble exists, but is not given as a option to plot due to it having an invalid experiment)

(This is when responses.json is deleted and we open manage-experiment. The experiment is shown, but cannot be selected. It is greyed out, and gives error as a tooltip when hovered)

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-18T14:06:15Z

CodSpeed Performance Report

Merging #9589 will not alter performance

_{Comparing jonathan-eq:fix-missing_responses_error (5226e19) with main (c43f97f)}

Summary

✅ 24 untouched benchmarks

codecov-commenter · 2024-12-18T15:02:45Z

Codecov Report

Attention: Patch coverage is 80.00000% with 15 lines in your changes missing coverage. Please review.

Project coverage is 91.83%. Comparing base (01156bb) to head (869f845).
Report is 20 commits behind head on main.

Files with missing lines	Patch %	Lines
.../ert/gui/tools/manage_experiments/storage_model.py	77.77%	6 Missing ⚠️
src/ert/gui/ertwidgets/ensembleselector.py	66.66%	4 Missing ⚠️
src/ert/storage/local_experiment.py	82.35%	3 Missing ⚠️
src/ert/storage/local_ensemble.py	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9589      +/-   ##
==========================================
- Coverage   91.85%   91.83%   -0.03%     
==========================================
  Files         433      433              
  Lines       26768    26879     +111     
==========================================
+ Hits        24587    24683      +96     
- Misses       2181     2196      +15

Flag	Coverage Δ
cli-tests	`39.69% <22.66%> (-0.07%)`	⬇️
everest-models-test	`34.55% <18.66%> (-0.04%)`	⬇️
gui-tests	`72.13% <77.33%> (+0.02%)`	⬆️
integration-test	`38.71% <22.66%> (+1.51%)`	⬆️
performance-tests	`51.88% <36.00%> (-0.06%)`	⬇️
test	`40.02% <22.66%> (-0.43%)`	⬇️
unit-tests	`74.06% <48.00%> (-0.11%)`	⬇️

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.

src/ert/gui/tools/manage_experiments/storage_model.py

andreas-el · 2025-01-08T10:58:27Z

src/ert/gui/ertnotifier.py

@@ -63,3 +64,10 @@ def set_current_ensemble(self, ensemble: Ensemble | None = None) -> None:
    @Slot(bool)
    def set_is_simulation_running(self, is_running: bool) -> None:
        self._is_simulation_running = is_running
+
+    def refresh(self):


Add None return

andreas-el · 2025-01-08T11:03:54Z

src/ert/gui/ertwidgets/ensembleselector.py

+                and not ensemble.experiment.is_valid()
+            ):
+                index = self.count() - 1
+                model_item = model.item(index)


Does this work? Not itemData(index) ?
https://doc.qt.io/qtforpython-5/PySide2/QtCore/QAbstractItemModel.html#PySide2.QtCore.PySide2.QtCore.QAbstractItemModel.itemData
https://doc.qt.io/qt-6/qabstractitemmodel.html#itemData

Yes, it works well. Should I use ItemData instead?

src/ert/gui/tools/manage_experiments/storage_model.py

andreas-el · 2025-01-08T11:08:29Z

src/ert/gui/tools/manage_experiments/storage_model.py

+            return default_flags
+        item = index.internalPointer()
+        if isinstance(item, ExperimentModel) and not item._is_valid:
+            return default_flags & ~Qt.ItemFlag.ItemIsEnabled


Could the be resolved if you create a typed variable and apply the flag change?

I had to wrap the itemflag in ItemFlags, and then mypy was happy :)

jonathan-eq · 2025-01-13T12:53:58Z

src/ert/gui/tools/manage_experiments/storage_widget.py

@@ -108,7 +111,7 @@ def __init__(
        search_bar.setPlaceholderText("Filter")
        proxy_model = _SortingProxyModel(storage_model)
        proxy_model.setFilterKeyColumn(-1)  # Search all columns.
-        proxy_model.setSourceModel(storage_model)
+        proxy_model.setSourceModel(storage_model)  # JONAK - CAN THIS BE REMOVED?


This requires a bigger rewrite of the GUI, so it will be in a separate PR.

jonathan-eq · 2025-01-14T12:51:46Z

src/ert/gui/main_window.py

@@ -154,6 +154,7 @@ def right_clicked(self) -> None:
    def select_central_widget(self) -> None:
        actor = self.sender()
        if actor:
+            self.notifier.refresh()


This refreshes storage to make sure the experiment files (responses, index, metadata, and parameters) are still valid.

jonathan-eq · 2025-01-14T13:36:02Z

There is a bug where it crashes if you are already in the manage experiment window, tab out to delete the responses.json file, and then put focus back on the ert window. I am trying to have ert refresh storage when gaining focus, but reimplementing focusInEvent on the main window does not work (it is never called). Is there a way I can make this work so that I can force refresh/revalidate storage before the child widgets start selecting invalid experiments @andreas-el?

The handling of the missing file should in the future be extended to also handle the other experiment files in a similar manner (index.json, metadata.json, and parameters.json) This commit: * Makes storage reload and re-validate when changing between ert modes (manage experient, run experiment, and plot tool) * Disables ensemble with invalid experiments from ensemble_selectors (error is shown as tooltip on hover) * Filters out invalid experiments from dark storage, so plotter won't attempt to plot them. * Reloads and re-validates storage on end of experiment, so ert won't crash if responses.json is deleted mid-run.

jonathan-eq force-pushed the fix-missing_responses_error branch from d644aee to 2fe1d74 Compare December 18, 2024 14:30

andreas-el reviewed Jan 8, 2025

View reviewed changes

src/ert/gui/tools/manage_experiments/storage_model.py Outdated Show resolved Hide resolved

andreas-el reviewed Jan 8, 2025

View reviewed changes

src/ert/gui/tools/manage_experiments/storage_model.py Outdated Show resolved Hide resolved

andreas-el reviewed Jan 8, 2025

View reviewed changes

jonathan-eq commented Jan 13, 2025

View reviewed changes

jonathan-eq force-pushed the fix-missing_responses_error branch 2 times, most recently from abb616c to d6e1bdd Compare January 14, 2025 08:55

jonathan-eq commented Jan 14, 2025

View reviewed changes

jonathan-eq added 2 commits January 14, 2025 14:46

Try having QMainWindow refresh storage when gaining focus

5226e19

jonathan-eq force-pushed the fix-missing_responses_error branch from 4658d21 to 5226e19 Compare January 14, 2025 13:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling of missing responses.json #9589

Improve handling of missing responses.json #9589

jonathan-eq commented Dec 18, 2024 •

edited

Loading

codspeed-hq bot commented Dec 18, 2024 •

edited

Loading

codecov-commenter commented Dec 18, 2024 •

edited

Loading

andreas-el Jan 8, 2025

andreas-el Jan 8, 2025

jonathan-eq Jan 14, 2025

andreas-el Jan 8, 2025

jonathan-eq Jan 14, 2025

jonathan-eq Jan 13, 2025

jonathan-eq Jan 14, 2025

jonathan-eq commented Jan 14, 2025

Improve handling of missing responses.json #9589

Are you sure you want to change the base?

Improve handling of missing responses.json #9589

Conversation

jonathan-eq commented Dec 18, 2024 • edited Loading

When applicable

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

Merging #9589 will not alter performance

Summary

codecov-commenter commented Dec 18, 2024 • edited Loading

Codecov Report

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 commented Jan 14, 2025

jonathan-eq commented Dec 18, 2024 •

edited

Loading

codspeed-hq bot commented Dec 18, 2024 •

edited

Loading

codecov-commenter commented Dec 18, 2024 •

edited

Loading