Account for ert and ert3 discrepancy when uploading parameter data to storage #192

DanSava · 2021-11-23T13:13:14Z

Issue
Resolves: #191

It seems parameters are handled differently for ert3 and ert at the moment:

Ert3: uploads a record for each realisation index.
Ert: uploads only one global record for each parameter.

Approach

For a given parameter (i.e. coefficients) try to retrieve a global record (assuming the user has run ERT) if this fails for the same parameter try to retrieve a record for a realization index in the list of active realization of the run example.

markusdregi · 2021-11-24T07:19:54Z

I did some testing of this PR and I can confirm that the visualiser no longer crashes 👍🏻

First, a comment on the behaviour of the visualiser: coefficients consists of three different variables, namely a, b and c. I believe that earlier I've been able to plot these independently and this no longer seems possible...

Second, a comment on the approach: I'm not entirely convinced that this should be resolved in webviz-ert given the apparent reason for the problem. If ert-storage serves the records differently dependent on whether they where uploaded in one batch or one by one I would perhaps consider that an ert-storage bug?

mortalisk · 2021-12-09T11:02:21Z

webviz_ert/data_loader/__init__.py

+            return df
+        except DataLoaderException as e:
+            logger.error(e)
+            return self.get_ensemble_record_data(


Is this necessary because the above only fetches ensemble wide data, and this call fetches realization by realization?

Yes, that's right

frode-aarstad

LGTM

markusdregi · 2021-12-16T14:15:55Z

webviz_ert/models/ensemble_model.py

@@ -98,7 +101,18 @@ def parameters(
        self,
    ) -> Optional[Mapping[str, ParametersModel]]:
        if not self._parameters:
-            parameter_names = self._data_loader.get_ensemble_parameters(self._id)
+            parameter_names = []
+            # TODO Remove except code path after PR https://github.com/equinor/ert-storage/pull/180 is merged


I think TODO's are more likely to be picked up in issues, then in the code ;)

Generally, that is right but the point of this TODO was to remind me that there is still something to be solved and removed before having a final version of the code and merging it.... like a TODO is meant to be right 🤔 ?

Ah, so this is not ready for merging yet then? :)

It is now that the TODO was dealt with also jenkins should also pass once the new ert-storage release is in bleeding

markusdregi · 2021-12-16T14:29:18Z

I'm pressuming this is not the end of this effort? Would be nice to see the issues pointing laying out the remaining work before we merge this :)

… storage.

DanSava · 2021-12-16T15:28:29Z

I'm pressuming this is not the end of this effort

Not really sure what "this effort" is that you are hinting at.

This PR should enable webviz ert to properly display grouped ert3 parameters and plot them also get responses as ensemble wide records.

DanSava · 2021-12-17T07:02:48Z

Jenkins test this please

markusdregi · 2021-12-20T10:44:23Z

webviz_ert/data_loader/__init__.py

@@ -113,7 +113,7 @@ class DataLoader:
    _instances: MutableMapping[ServerIdentifier, "DataLoader"] = {}

    baseurl: str
-    token: str
+    token: Optional[str]


Could you explain why this is optional now?

Not really sure why pylint was not complaining before about this, it just started, I guess it was updated recently.

The Dataloader __new__ definition looked like this for quite some time

def __new__(cls, baseurl: str, token: Optional[str] = None) -> "DataLoader":

So token is expected to he Optional[str]

markusdregi · 2021-12-20T10:45:52Z

webviz_ert/data_loader/__init__.py

-        df = pd.read_parquet(stream)
-        return df
+        try:
+            if "::" in parameter_name:


This is what I meant by future work. It might be that I'm misunderstanding something, but is this not somewhat of a hack to avoid having to make ERT2 upload parameters as it should?

Sure we can make ert2 upload parameters like ert3 does it, but I thought the idea was that the underlying Data structure is the same for both ert3 and ert2 it should not matter how they are uploaded, ert2 or ert3 style given that both ways are valid and supported by the storage API.

markusdregi

I was expecting some tests verifying the current :: logic...?

markusdregi · 2021-12-20T10:53:28Z

I did some testing and it seems like the original problems are fixed 👍🏻 I tried running the entire polynomial case with ert3 using run_demo and it crashes when I'm trying to visualise the doe experiment with

raise DataLoaderException(
webviz_ert.data_loader.DataLoaderException: Error fetching data from http://127.0.0.1:51822/ensembles/12d84e2f-7ccc-4e95-bd1a-328c0658b962/records/coefficients/labels
                The request return with status code: 404
                b'{"detail":{"ensemble_id":"12d84e2f-7ccc-4e95-bd1a-328c0658b962","name":"coefficients","error":"Record not found"}}'

I guess we should either fix it here or create a new issue for it. I'm fine with either :)

DanSava · 2021-12-20T11:42:42Z

I did some testing and it seems like the original problems are fixed 👍🏻 I tried running the entire polynomial case with ert3 using run_demo and it crashes when I'm trying to visualise the doe experiment with
raise DataLoaderException(
webviz_ert.data_loader.DataLoaderException: Error fetching data from http://127.0.0.1:51822/ensembles/12d84e2f-7ccc-4e95-bd1a-328c0658b962/records/coefficients/labels
                The request return with status code: 404
                b'{"detail":{"ensemble_id":"12d84e2f-7ccc-4e95-bd1a-328c0658b962","name":"coefficients","error":"Record not found"}}'
I guess we should either fix it here or create a new issue for it. I'm fine with either :)

I think a new issue is required, I will create it. I think this never worked (the visualisation) it cannot find the coefficient record because it is not part of the doe ensemble due to it being added to storage as part of the "general" ensemble.

The coefficients are added to storage before the experiment is run with ert3 record load designed_coefficients experiments/doe/coefficients_record.json

DanSava · 2021-12-20T12:42:11Z

#203 was added to track the issue reported ⬆️

markusdregi

Thanks for the update 👍🏻

DanSava self-assigned this Nov 23, 2021

DanSava force-pushed the fix_ert3_parameter_vis branch from 2bea1ae to 1a22ee8 Compare November 23, 2021 13:19

DanSava force-pushed the fix_ert3_parameter_vis branch 5 times, most recently from d37df69 to 37e9f3a Compare December 1, 2021 12:33

mortalisk reviewed Dec 9, 2021

View reviewed changes

DanSava force-pushed the fix_ert3_parameter_vis branch 3 times, most recently from 874ece5 to 19df739 Compare December 15, 2021 10:38

frode-aarstad approved these changes Dec 16, 2021

View reviewed changes

markusdregi reviewed Dec 16, 2021

View reviewed changes

DanSava force-pushed the fix_ert3_parameter_vis branch 2 times, most recently from 8f7864d to d0194f5 Compare December 16, 2021 14:47

markusdregi marked this pull request as draft December 16, 2021 14:52

Account for ert and ert3 discrepancy when uploading parameter data to…

00a3826

… storage.

DanSava force-pushed the fix_ert3_parameter_vis branch from d0194f5 to 00a3826 Compare December 16, 2021 15:20

DanSava marked this pull request as ready for review December 16, 2021 15:22

markusdregi reviewed Dec 20, 2021

View reviewed changes

markusdregi suggested changes Dec 20, 2021

View reviewed changes

Add tests for labeled parameters

c5b3723

DanSava force-pushed the fix_ert3_parameter_vis branch from 8f23b8e to c5b3723 Compare December 21, 2021 10:48

DanSava requested a review from markusdregi December 21, 2021 10:52

markusdregi approved these changes Dec 22, 2021

View reviewed changes

DanSava merged commit d6c4a41 into equinor:master Dec 22, 2021

DanSava deleted the fix_ert3_parameter_vis branch December 22, 2021 13:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account for ert and ert3 discrepancy when uploading parameter data to storage #192

Account for ert and ert3 discrepancy when uploading parameter data to storage #192

DanSava commented Nov 23, 2021 •

edited

Loading

markusdregi commented Nov 24, 2021

mortalisk Dec 9, 2021

DanSava Dec 14, 2021

frode-aarstad left a comment

markusdregi Dec 16, 2021

DanSava Dec 16, 2021

markusdregi Dec 16, 2021

DanSava Dec 16, 2021 •

edited

Loading

markusdregi commented Dec 16, 2021

DanSava commented Dec 16, 2021

DanSava commented Dec 17, 2021

markusdregi Dec 20, 2021

DanSava Dec 20, 2021

markusdregi Dec 20, 2021

markusdregi Dec 20, 2021

DanSava Dec 20, 2021

markusdregi left a comment

markusdregi commented Dec 20, 2021

DanSava commented Dec 20, 2021

DanSava commented Dec 20, 2021

markusdregi left a comment

Account for ert and ert3 discrepancy when uploading parameter data to storage #192

Account for ert and ert3 discrepancy when uploading parameter data to storage #192

Conversation

DanSava commented Nov 23, 2021 • edited Loading

markusdregi commented Nov 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frode-aarstad left a 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

DanSava Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

markusdregi commented Dec 16, 2021

DanSava commented Dec 16, 2021

DanSava commented Dec 17, 2021

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

markusdregi left a comment

Choose a reason for hiding this comment

markusdregi commented Dec 20, 2021

DanSava commented Dec 20, 2021

DanSava commented Dec 20, 2021

markusdregi left a comment

Choose a reason for hiding this comment

DanSava commented Nov 23, 2021 •

edited

Loading

DanSava Dec 16, 2021 •

edited

Loading