-
Notifications
You must be signed in to change notification settings - Fork 110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Have forward_model_ok
not run if storage in invalid (missing responses.json)
#9857
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ | |
from pathlib import Path | ||
|
||
from ert.config import InvalidResponseFile | ||
from ert.storage import Ensemble | ||
from ert.storage import Ensemble, ErtStorageException | ||
from ert.storage.realization_storage_state import RealizationStorageState | ||
|
||
from .load_status import LoadResult, LoadStatus | ||
|
@@ -96,36 +96,49 @@ async def forward_model_ok( | |
realization: int, | ||
iter: int, | ||
ensemble: Ensemble, | ||
forward_model_ok_permanent_error_future: asyncio.Future[str] | None = None, | ||
) -> LoadResult: | ||
parameters_result = LoadResult(LoadStatus.LOAD_SUCCESSFUL, "") | ||
response_result = LoadResult(LoadStatus.LOAD_SUCCESSFUL, "") | ||
try: | ||
# We only read parameters after the prior, after that, ERT | ||
# handles parameters | ||
if iter == 0: | ||
parameters_result = await _read_parameters( | ||
run_path, | ||
realization, | ||
iter, | ||
ensemble, | ||
) | ||
|
||
if parameters_result.status == LoadStatus.LOAD_SUCCESSFUL: | ||
response_result = await _write_responses_to_storage( | ||
run_path, | ||
realization, | ||
ensemble, | ||
) | ||
|
||
except Exception as err: | ||
logger.exception( | ||
f"Failed to load results for realization {realization}", | ||
exc_info=err, | ||
) | ||
if forward_model_ok_permanent_error_future is None: | ||
forward_model_ok_permanent_error_future = asyncio.Future() | ||
if ( | ||
forward_model_ok_permanent_error_future.done() | ||
and forward_model_ok_permanent_error_future.exception() | ||
): | ||
parameters_result = LoadResult( | ||
LoadStatus.LOAD_FAILURE, | ||
f"Failed to load results for realization {realization}, failed with: {err}", | ||
f"Failed to load results for realization {realization}, failed with: {forward_model_ok_permanent_error_future.exception()}", | ||
) | ||
else: | ||
try: | ||
# We only read parameters after the prior, after that, ERT | ||
# handles parameters | ||
if iter == 0: | ||
parameters_result = await _read_parameters( | ||
run_path, | ||
realization, | ||
iter, | ||
ensemble, | ||
) | ||
|
||
if parameters_result.status == LoadStatus.LOAD_SUCCESSFUL: | ||
response_result = await _write_responses_to_storage( | ||
run_path, | ||
realization, | ||
ensemble, | ||
) | ||
except Exception as err: | ||
if isinstance(err, ErtStorageException): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ErtStorageException does not just signify that the storage mount point does not exist, but many other errors for which forward_model_ok might succeed. |
||
forward_model_ok_permanent_error_future.set_exception(err) | ||
logger.exception( | ||
f"Failed to load results for realization {realization}", | ||
exc_info=err, | ||
) | ||
parameters_result = LoadResult( | ||
LoadStatus.LOAD_FAILURE, | ||
f"Failed to load results for realization {realization}, failed with: {err}", | ||
) | ||
|
||
final_result = parameters_result | ||
if response_result.status != LoadStatus.LOAD_SUCCESSFUL: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ | |
from typing_extensions import deprecated | ||
|
||
from ert.config.gen_kw_config import GenKwConfig | ||
from ert.storage.local_experiment import ErtStorageException | ||
from ert.storage.mode import BaseMode, Mode, require_write | ||
|
||
from .realization_storage_state import RealizationStorageState | ||
|
@@ -406,8 +407,10 @@ def get_ensemble_state(self) -> list[set[RealizationStorageState]]: | |
states : list of RealizationStorageState | ||
list of realization states. | ||
""" | ||
|
||
response_configs = self.experiment.response_configuration | ||
try: | ||
response_configs = self.experiment.response_configuration | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does not immediately seem fine to ignore a storage exception here. |
||
except ErtStorageException: | ||
response_configs = {} | ||
|
||
def _parameters_exist_for_realization(realization: int) -> bool: | ||
""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,10 @@ class _Index(BaseModel): | |
name: str | ||
|
||
|
||
class ErtStorageException(Exception): | ||
pass | ||
|
||
|
||
class LocalExperiment(BaseMode): | ||
""" | ||
Represents an experiment within the local storage system of ERT. | ||
|
@@ -253,6 +257,10 @@ def parameter_info(self) -> dict[str, Any]: | |
@property | ||
def response_info(self) -> dict[str, Any]: | ||
info: dict[str, Any] | ||
if not (self.mount_point / self._responses_file).exists(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was just changed to give a |
||
raise ErtStorageException( | ||
"responses.json does not exist. Please make sure storage is still valid." | ||
) | ||
with open(self.mount_point / self._responses_file, encoding="utf-8") as f: | ||
info = json.load(f) | ||
return info | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how the Future is usually used is that when you do somewhere:
and somewhere else you get exception Ex you can propagate it to the future by
which then gets triggered by the line above (with await).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not want to await it, as it might never be set. I only use it so that multiple tasks can set the value, and the others can check if that value has already been set, meaning they should halt. It worked the same if I used myAsyncioFuture.set_result(reason_why_it_failed) or myAsyncioFuture.set_exception(exception_why_it_failed).
I will change it to use the latter one, as I already have the raw exception available.