Test ensemble_evaluator with new scheduler #6803

berland · 2023-12-14T08:17:21Z

Adds testing and fixes some problems.

Currently it allows for a change in events emitted by the local driver from legacy to new. This is up for discussion.

Should be merged after #6787 as code is stolen therefrom

Approach
Test unit_tests/ensemble_evaluator with scheduler using new fixture.

Pre review checklist

Read through the code changes carefully after finishing work
Make sure tests pass locally (after every commit!)
Prepare changes in small commits for more convenient review (optional)
PR title captures the intent of the changes, and is fitting for release notes.
Updated documentation
Ensured that unit tests are added for all new behavior (See
Ground Rules),
and changes to existing code have good test coverage.

Pre merge checklist

Added appropriate release note label
Commit history is consistent and clean, in line with the contribution guidelines.

codecov-commenter · 2023-12-14T08:29:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0140ccc) 83.90% compared to head (f00c196) 83.90%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6803   +/-   ##
=======================================
  Coverage   83.90%   83.90%           
=======================================
  Files         365      365           
  Lines       21410    21410           
  Branches      948      948           
=======================================
  Hits        17964    17964           
  Misses       3152     3152           
  Partials      294      294

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

berland · 2023-12-14T09:10:10Z

tests/unit_tests/ensemble_evaluator/test_async_queue_execution.py

-    end_event = from_json(mock_ws_task.result()[end_event_index])
-    assert end_event["type"] == "com.equinor.ert.realization.success"
-    assert end_event.data == {"queue_event_type": "SUCCESS"}
+    if FeatureToggling.is_enabled("scheduler"):


Look here for the change of events emitted by the new local driver. Also note that "waiting" is translated to "SUBMITTED", but not for the legacy driver/job_queue_node(?).

I think this is because the initial state is assumed to be WAITING in Scheduler, so the first new event is SUBMITTED, while JobQueue starts with NOT_ACTIVE, so the first event is WAITING.

I don't see the need for two different states that both say "we're not doing anything yet". Maybe we should consider removing NOT_ACTIVE from JobQueue to avoid this if? Alternatively, Scheduler might send WAITING explicitly.

I am fine with differing behaviour for now, so we can reduce the number of states while transitioning to Scheduler. Lets reconsider if a case pops up where we need to distuingish between NOT_ACTIVE and WAITING in Scheduler.

pinkwah · 2023-12-14T09:44:58Z

src/ert/scheduler/job.py

@@ -104,7 +104,7 @@ async def _send(self, state: State) -> None:
        event = CloudEvent(
            {
                "type": _queue_state_event_type(status),
-                "source": f"/etc/ensemble/{self._scheduler._ens_id}/real/{self.iens}",
+                "source": f"/ert/ensemble/{self._scheduler._ens_id}/real/{self.iens}",


No way did I really write etc and not notice??? 😅

Your finger muscle memory is forgiven

pinkwah · 2023-12-20T13:28:43Z

src/ert/scheduler/job.py

@@ -73,7 +73,7 @@ async def _submit_and_run_once(self, sem: asyncio.BoundedSemaphore) -> None:
                self.real.iens, self.real.job_script, cwd=self.real.run_arg.runpath
            )

-            await self._send(State.STARTING)
+            await self._send(State.STARTING)  # aka PENDING


Is PENDING a better name? Should we change it back so that we don't need this comment?

Perhaps not better by itself, but more familiar for legacy ert devs..

Will change back to PENDING. I will claim it means exactly the same, and also makes as much sense as STARTING for the LocalDriver.

pinkwah · 2023-12-20T13:32:49Z

tests/unit_tests/ensemble_evaluator/test_async_queue_execution.py

-    end_event = from_json(mock_ws_task.result()[end_event_index])
-    assert end_event["type"] == "com.equinor.ert.realization.success"
-    assert end_event.data == {"queue_event_type": "SUCCESS"}
+    if FeatureToggling.is_enabled("scheduler"):


I think this is because the initial state is assumed to be WAITING in Scheduler, so the first new event is SUBMITTED, while JobQueue starts with NOT_ACTIVE, so the first event is WAITING.

I don't see the need for two different states that both say "we're not doing anything yet". Maybe we should consider removing NOT_ACTIVE from JobQueue to avoid this if? Alternatively, Scheduler might send WAITING explicitly.

This highlights a behavioural change in the new LocalDriver, it will not send the same events as the legacy local driver, see test_async_queue_execution.py::test_happy_path The new scheduler will not catch bare exceptions for now, and thus the test for that situation is only applied for the legacy JobQueue.

berland · 2023-12-22T13:38:26Z

tests/unit_tests/ensemble_evaluator/test_ensemble_legacy.py

+    tmpdir, make_ensemble_builder, monkeypatch
+):
+    """This test function is not ported to Scheduler, as it will not
+    catch general exceptions."""


The logs have been searched through, and the general exception that the legacy queue can catch has not happened the last 3 months.

pinkwah

💯

berland self-assigned this Dec 14, 2023

berland added scheduler release-notes:skip If there should be no mention of this in release notes labels Dec 14, 2023

berland marked this pull request as ready for review December 14, 2023 09:08

berland commented Dec 14, 2023

View reviewed changes

pinkwah reviewed Dec 14, 2023

View reviewed changes

berland force-pushed the test_ens_ev_scheduler branch from 3b5aad0 to 87eae99 Compare December 14, 2023 13:43

berland mentioned this pull request Dec 18, 2023

Verify existing unit_tests with new Scheduler #6810

Closed

35 tasks

berland force-pushed the test_ens_ev_scheduler branch 3 times, most recently from 50318a2 to 326edeb Compare December 20, 2023 12:21

berland enabled auto-merge (rebase) December 20, 2023 13:25

pinkwah reviewed Dec 20, 2023

View reviewed changes

berland force-pushed the test_ens_ev_scheduler branch 2 times, most recently from 26a222f to cd3d4d2 Compare December 22, 2023 07:35

berland force-pushed the test_ens_ev_scheduler branch from cd3d4d2 to f00c196 Compare December 22, 2023 13:23

berland commented Dec 22, 2023

View reviewed changes

pinkwah approved these changes Dec 22, 2023

View reviewed changes

berland merged commit e9ff239 into equinor:main Dec 22, 2023
42 of 43 checks passed

berland deleted the test_ens_ev_scheduler branch June 6, 2024 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test ensemble_evaluator with new scheduler #6803

Test ensemble_evaluator with new scheduler #6803

berland commented Dec 14, 2023 •

edited

Loading

codecov-commenter commented Dec 14, 2023 •

edited

Loading

berland Dec 14, 2023

pinkwah Dec 20, 2023

berland Dec 20, 2023

pinkwah Dec 14, 2023

berland Dec 14, 2023

pinkwah Dec 20, 2023

berland Dec 20, 2023

berland Dec 20, 2023

pinkwah Dec 20, 2023

berland Dec 22, 2023

pinkwah left a comment

Test ensemble_evaluator with new scheduler #6803

Test ensemble_evaluator with new scheduler #6803

Conversation

berland commented Dec 14, 2023 • edited Loading

Pre review checklist

Pre merge checklist

codecov-commenter commented Dec 14, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinkwah left a comment

Choose a reason for hiding this comment

berland commented Dec 14, 2023 •

edited

Loading

codecov-commenter commented Dec 14, 2023 •

edited

Loading