Add opt-in for deleting intermediate runpaths for es_mda - #14325
Conversation
ce33ecd to
eec41c3
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds optional ES-MDA intermediate runpath cleanup to reduce scratch-space usage.
Changes:
- Adds GUI and CLI controls for cleanup.
- Deletes intermediate ES-MDA runpaths after evaluation.
- Persists and tests the new configuration field.
File summaries
| File | Description |
|---|---|
src/ert/__main__.py |
Adds the CLI cleanup flag. |
src/ert/gui/experiments/multiple_data_assimilation_panel.py |
Adds the default-enabled GUI checkbox. |
src/ert/run_models/model_factory.py |
Passes cleanup configuration to the model. |
src/ert/run_models/multiple_data_assimilation.py |
Implements intermediate runpath deletion. |
src/ert/run_models/run_model_configs.py |
Adds and serializes the cleanup setting. |
src/ert/storage/local_experiment.py |
Extends stored experiment metadata. |
tests/ert/unit_tests/run_models/test_multiple_data_assimilation.py |
Tests runpath deletion behavior. |
tests/ert/unit_tests/run_models/test_model_factory.py |
Updates ES-MDA factory fixtures. |
tests/ert/unit_tests/run_models/test_experiment_serialization.py |
Updates serialization input. |
tests/.../snake_oil.json |
Updates the ES-MDA snapshot. |
tests/.../poly.json |
Updates the ES-MDA snapshot. |
tests/.../config.json |
Updates the ES-MDA snapshot. |
Review details
Suppressed comments (2)
src/ert/run_models/multiple_data_assimilation.py:231
ignore_errors=Truesilently suppresses permission and filesystem failures, so the run can claim it is deleting paths while freeing no scratch space. CatchOSErrorand emit a warning (without failing the expensive experiment) so operators can detect cleanup failures.
logger.info(f"Deleting runpath of intermediate iteration: {path}")
shutil.rmtree(path, ignore_errors=True)
src/ert/gui/experiments/multiple_data_assimilation_panel.py:233
- This is a new user-facing, default-enabled destructive behavior, but no
.rstdocumentation is updated. Document what is removed, that storage results remain, how GUI users opt out, and that the CLI remains opt-in; the existing ES-MDA how-to atdocs/ert/getting_started/howto/esmda_select_prior.rstis a natural location.
layout.addRow(
("Only keep prior and\nposterior on disk:"),
self._only_keep_first_and_last_iteration_box,
)
- Files reviewed: 12/12 changed files
- Comments generated: 6
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14325 +/- ##
==========================================
- Coverage 90.81% 90.74% -0.07%
==========================================
Files 497 497
Lines 36062 36093 +31
==========================================
+ Hits 32748 32751 +3
- Misses 3314 3342 +28
Flags with carried forward coverage won't be shown. Click here to find out more.
|
2897dfb to
b11c74b
Compare
b11c74b to
dd5f9b6
Compare
|
Screenshots differ from baselines. A baseline update PR has been prepared: equinor/ert-testdata#112 |
dd5f9b6 to
4766b08
Compare
es_mda
77ad712 to
d248a52
Compare
| for path in self._run_paths.get_paths( | ||
| realizations_stored_successfully, posterior.iteration | ||
| ): | ||
| logger.info(f"Deleting runpath of intermediate iteration: {path}") | ||
| shutil.rmtree(path, ignore_errors=True) |
There was a problem hiding this comment.
Interesting comment from Copilot, it is absolutely possible to set
RUNPATH poly_out/realiation-<IENS>/iter-foo
and es-mda will actually run quite fine, reusing the the runpath for each realization over the iterations, and you have effectively implicitly deleted iter-0 to iter-2. In this case the deletion feature is superfluous (but it does represent a safety net as rerunning in an existing realization can have side-effects, just not for iter-1) but I don't see any big problems with it.
d248a52 to
157fd8a
Compare
|
I am not sure about the usage of "intermediate runpath" in the GUI text here - wondering if users will understand that concept, and if they are unsure of what it means, they will for sure not click it. It may also be unclear to the user when this deletion is going to happen. We can calculate what the intermediate iterations are, based on the input further up in the GUI element, and include "iter-1, iter-2" in the text somehow. On-prem users would also like to see the term "scratch" in the message, they might relate more to that than the term "runpath", but then "scratch" is a specific user group tribe-language and not Ert lingo. |
I was thinking of wording the label in such a manner that it highlights that this is scratch disk optimization. However, my concern was that we could ruin runs for the users if we are not very explicit that this is a non-reversal deletion. The tooltip explains when the deletion happens, but I guess we could try to add this to the label too
I tried to reduce the amount of tribe-language used (I am still not consistent, e.g restart vs prediction), but in the user documentation I highlight that this frees up scratch disk space (selling point of functionality). I am not sure if we should accommodate wording or not, feel like we're currently in an awkward middle position:
Suggested labels:
|
157fd8a to
4dde255
Compare
|
|
||
|
|
||
| ES_MDA_DELETE_RUNPATHS_HELP_TEXT = ( | ||
| "Delete the runpath directories of intermediate iterations\n" |
There was a problem hiding this comment.
Instead of newlines, can't we have Qt or something wrap the text? The newlines make this text hard to maintain later.
There was a problem hiding this comment.
Text also gets used for the CLI-arg helper, unsure if there are methods to wrap the text there?
4dde255 to
0df6e3f
Compare
| "so failures can still be investigated.\n" | ||
| "Results are still available in storage,\n" | ||
| "but the deleted intermediate iterations can no longer\n" | ||
| "be inspected or used as a prior for a restart." |
There was a problem hiding this comment.
This last sentence is probably slightly incorrect. "Restart" is a very overloaded term (it is both an es-mda term, and an Eclipse term), and for some definitions (the Eclipse one), this statement is very correct. But since the parameters are in storage, es-mda can probably restart from intermediate iterations.
There was a problem hiding this comment.
If it can be used as prior despite being removed from scratch, should we also add opt-in to remove initial prior (iter-0)?
|
Screenshot tests now pass. The baseline update PR equinor/ert-testdata#112 has been closed. |
|
Another label suggestion: "Keep only prior and posteriors runpaths" |
| **self._update_experiment_config(), | ||
| **self._common_fields(), | ||
| "prior_ensemble_id": self.prior_ensemble_id, | ||
| "delete_intermediate_runpaths": (self.delete_intermediate_runpaths), |
There was a problem hiding this comment.
Does this belong here? Could see this belonging with the rest of the runpath configuration. While only needed in esmda at the moment, we do aim for this to be available and used for all run models down the line.
There was a problem hiding this comment.
If not added to the MultipleDataAssimilationConfig the experiment serialization test fails with:
AssertionError: Key delete_intermediate_runpaths not found in saved experiment. Expected value: False
Test added in this commit
Edit: Post discussion, will look into adding this to runpath config
Added a checkbox to run panel and cli arg for deleting intermediate runpath iterations Defaults to false, as the choice is irreversible Failed realization does not get deleted as they are removed from active realizations prior to the deletion method-call
0df6e3f to
a59a9ed
Compare

Added the option to delete the intermediate runpaths for
es_mda. Often not not used for prediction runs, QC should be able to be done with the data from storage. Will free up space on scratch.git rebase -i main --exec 'just rapid-tests')When applicable
merge screenshot-PR in ert-testdata before merging this PR.