Make sure that the last evaluator event is EETerminated #8887
Conversation
| async for event in monitor.track(): | ||
| if isinstance(event, EETerminated): |
There was a problem hiding this comment.
Can we check if the ensemble_state has changed appropriately? Otherwise this looks like an improvement.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8887 +/- ##
==========================================
+ Coverage 91.48% 91.49% +0.01%
==========================================
Files 345 345
Lines 21263 21267 +4
==========================================
+ Hits 19452 19459 +7
+ Misses 1811 1808 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| self._complete_batch.clear() | ||
| try: | ||
| event = await asyncio.wait_for(self._events.get(), timeout=0.1) | ||
| function = event_handler[type(event)] | ||
| batch.append((function, event)) | ||
| self._events.task_done() | ||
| except asyncio.TimeoutError: | ||
| continue | ||
| self._complete_batch.set() |
There was a problem hiding this comment.
Would it be over engineered if we made everything inside the loop a separate method, and had the self._complete_batch.set/clear in a decorator on that new method?
There was a problem hiding this comment.
I think it would be over engineered - yes! If there would be more utilization of such a decorator then I would say yes.
| await self._events.join() | ||
| await self._complete_batch.wait() |
| if type(event) in [EESnapshot, EESnapshotUpdate]: | ||
| assert finished_event_received == False |
There was a problem hiding this comment.
I think we should rename this variable to final_event_was_EETerminated
| @@ -469,11 +471,18 @@ async def test_ensure_multi_level_events_in_order(evaluator_to_use): | |||
| # about realizations, the state of the ensemble up until that point | |||
| # should be not final (i.e. not cancelled, stopped, failed). | |||
| ensemble_state = snapshot_event.snapshot.get("status") | |||
| final_event_was_EETerminated = False | |||
There was a problem hiding this comment.
Can we also have another flag for testing that we get both a snapshot/snapshotupdate in addition to the terminated event?
This introduces EnsembleEvaluator._complete_batch event, which is set every time a batch is produced. When closing the ee server, we make sure that the _complete_batch is set and thus preventing an events to be lost. This behaviour is then tested in test_ensure_multi_level_events_in_order, which was updated correspondighly.
Issue
Resolves #8869
Resolves #8889
Approach
Check that the last event in test_ensure_multi_level_events_in_order is EETerminated.
(Screenshot of new behavior in GUI if applicable)
git rebase -i main --exec 'pytest tests/ert/unit_tests -n logical -m "not integration_test"')When applicable