Type and style for ensemble evaluator builder code #3219
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jondequinor
added
enhancement
maintenance
Codecov Report
@@ Coverage Diff @@
## main #3219 +/- ##
==========================================
- Coverage 65.60% 65.58% -0.02%
==========================================
Files 614 619 +5
Lines 48982 49059 +77
Branches 4407 4407
==========================================
+ Hits 32135 32177 +42
- Misses 15374 15412 +38
+ Partials 1473 1470 -3
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
jondequinor
force-pushed
the
ee-builder-typing
branch
from
1250d3e
to
9aa2652
Compare
jondequinor
changed the title
jondequinor
force-pushed
the
ee-builder-typing
branch
from
9aa2652
to
e15f890
Compare
Move the ensemble evaluator builder code, along with some utility code, to ert/ and add type hinting and style fixes. Type hinting and style fixes have also been extended to code now under audit by virtue of the move to ert/. Some refactors, not strictly necessary, have been made to ease typing.
jondequinor
force-pushed
the
ee-builder-typing
branch
from
e15f890
to
8e59f9e
Compare
jondequinor
commented
Apr 8, 2022
Comment on lines
+2
to
+7
| from .builder._ensemble import _Ensemble | ||
| from .builder._ensemble_builder import _EnsembleBuilder | ||
| from .builder._io_ import _IO, _InputBuilder, _OutputBuilder | ||
| from .builder._job import _JobBuilder, _LegacyJobBuilder | ||
| from .builder._realization import _RealizationBuilder | ||
| from .builder._step import _StepBuilder |
There was a problem hiding this comment.
Find arguments for why this is done like so
jondequinor
commented
Apr 8, 2022
| from ._realization import _Realization | ||
|
|
||
| if TYPE_CHECKING: | ||
| import ert |
jondequinor
commented
Apr 8, 2022
| @@ -89,7 +97,10 @@ def _evaluate(self): | |||
| # Get a fresh eventloop | |||
| asyncio.set_event_loop(asyncio.new_event_loop()) | |||
|
|
|||
| async def _evaluate_inner(): | |||
| if self._config is None: | |||
| raise RuntimeError("no config") | |||
jondequinor
commented
Apr 8, 2022
Comment on lines
+144
to
+148
| for real in self.active_reals: | ||
| self._job_queue.add_ee_stage( # type: ignore | ||
| real.steps[0], callback_timeout=on_timeout | ||
| ) | ||
| self._job_queue.submit_complete() | ||
| self._job_queue.submit_complete() # type: ignore |
There was a problem hiding this comment.
Opted out of typing these.
jondequinor
commented
Apr 8, 2022
|
|
||
| finally: | ||
| await timeout_queue.put(None) # signal to exit timer | ||
| await send_timeout_future | ||
|
|
||
| # Dispatch final result from evaluator - FAILED, CANCEL or STOPPED | ||
| assert self._config # mypy |
There was a problem hiding this comment.
Question @sondreso : ValueError or assertion here?
There was a problem hiding this comment.
I think this is a ValueError
sondreso
approved these changes
Apr 8, 2022
|
Ahh, I did not see the auto merge 😬 |
sondreso
added
the
release-notes:maintenance
|
My bad. Will do a follow up |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue
Resolves #3188
Approach
Move the ensemble evaluator builder code, along with some utility code, to ert/ and add type hinting and style fixes. Type hinting and style fixes have also been extended to code now under audit by virtue of the move to ert/. Some refactors, not strictly necessary, have been made to ease typing.
A programming error was made that allowed unix jobs to overwrite output from other unix jobs, causing hard-to-debug test failures. A constraint on file transformations were introduced such that they cannot overwrite files from transforming from a record (to file).
For unknown reasons, I've made a flaky test more flaky. A piece of duct tape was applied. Proper fix will be Stop pickling RecordTransmitters and serialize its configuration instead #3013 and running prefect without multiprocessing.
Pre review checklist
Adding labels helps the maintainers when writing release notes. This is the list of release note labels.