Unify execute API of JobQueue, refactor websocket connection #6558
Conversation
berland
force-pushed
the
jobqueue_unify_execute_api
branch
4 times, most recently
from
d805142 to
41868ba
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #6558 +/- ##
==========================================
- Coverage 83.51% 83.51% -0.01%
==========================================
Files 346 346
Lines 20764 20778 +14
Branches 948 948
==========================================
+ Hits 17341 17352 +11
- Misses 3129 3132 +3
Partials 294 294 ☔ View full report in Codecov by Sentry. |
berland
force-pushed
the
jobqueue_unify_execute_api
branch
from
0c76269 to
ceb0b47
Compare
berland
changed the title
berland
added
the
release-notes:maintenance
|
Tested manually with GUI for poly_example, bigpoly and also with everest's egg model. |
|
Up for discussion: Should this be squashed? |
| ) -> None: | ||
| assert self.ens_id is not None # mypy | ||
| events = deque( |
There was a problem hiding this comment.
Why do we use deque here instead of a normal queue?
There was a problem hiding this comment.
probably a local and minor optimization thing to have a double-ended queue.
| if self._ee_uri is None: | ||
| # If no ensemble evaluator present, we will publish to the log | ||
| while ( | ||
| change := await self._changes_to_publish.get() |
There was a problem hiding this comment.
I did not know about this (walrus operator :=). Thanks🎉
| experiment_id: Optional[str] = None, | ||
| ) -> None: | ||
| for q_index, q_node in enumerate(self.job_list): | ||
| cert_path = f"{q_node.run_path}/{CERT_FILE}" | ||
| if cert is not None: | ||
| if self._ee_cert is not None: |
There was a problem hiding this comment.
Was it not clearer when conninfo was passed directly into the function as a parameter?
There was a problem hiding this comment.
It was more explicit, but that explicitness would f.ex make it possible to mixup the ee connection for the execute function with what is given in this jobs file. I think it is cleaner to set the connection info just one place in the Queue object.
There was a problem hiding this comment.
The only thing is that Queue does not need to know about connection setup between ee and job_runner; hence my comment on passing it rather as a function params 🤷
There was a problem hiding this comment.
That is correct, I am removing that "feature" to have different connection info.
I would squash the fixup commits at least :) |
The websocket connection from the JobQueue is refactored to use a single asyncio task for fetching changes to be published, publish them, and also keep the connection open. The queue execute function had two variants, one that included websocket communication (for "legacy" ert, and one for Everest, the "simulation context"). Now there is only one, which is async. The changes that are published to the websocket channel is in the simulation context (Everest) logged to the debug log. Previously that information was not propagated. The connection to the ensemble evaluator is now configured once, and from now on it is not possible to have the job_runner use a different connection than the JobQueue uses for publishing its realization state changes.
berland
force-pushed
the
jobqueue_unify_execute_api
branch
from
c327ea1 to
1ddc0c8
Compare
The simulation_context now has to call it in an async loop, and it is also doing slightly more work (logging changes) than before.
Pre review checklist
Ground Rules),
and changes to existing code have good test coverage.
Pre merge checklist