Skip to content
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

ENH: Add Ert experiment id to FMU metadata block #755

Merged

Conversation

slangeveld
Copy link
Contributor

@slangeveld slangeveld commented Aug 19, 2024

Resolves #670

Include _ERT_EXPERIMENT_ID from the enviroment variables in the FMU block of the exported metadata.

If the environment variable _ERT_EXPERIMENT_ID is present, it will be appended to the FMU block in the exported metadata. For this, a new "ert" field has been added to the metadata file under the FMU block, in addition to a child field "experiment", which will contain the _ERT_EXPERIMENT_ID of type UUID:

data:
  ...
fmu:
  case:
    name: ...
    uuid: ...
  ert:
    experiment:
      id: <_ERT_EXPERIMENT_ID>  #UUID

Two new tests have been added to test if the _ERT_EXPERIMENT_ID is part of the FMU metadata when it is present in the environment variables.

@mferrera mferrera requested a review from a team August 20, 2024 05:03
Copy link
Collaborator

@mferrera mferrera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of the group review: This field should be moved from FMUBase (which is used only in the the case metadata) to the FMU class, which is used in object metadata. Will need adjustment in the internal model too. But otherwise, looks good!

Comment on lines 562 to 564
ert: Optional[Ert] = Field(default=None)
"""The ``fmu.ert`` block contains information about the current ert run
object was exported. See :class:`Ert`."""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just move this 👍

@slangeveld slangeveld force-pushed the 670-add-ert_experiment_id-to-case_metadata branch from 6a50273 to 164517e Compare August 21, 2024 13:28
Copy link
Collaborator

@mferrera mferrera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good to go. @tnatt or @jcrivenaes should confirm before merging

Copy link
Collaborator

@tnatt tnatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is aligned with what we agreed upon 🚀 and LGTM 🙂

Comment on lines 183 to 188
ert=fields.Ert(
experiment=fields.Experiment(
id=uuid.UUID(FmuEnv.EXPERIMENT_ID.value)
if FmuEnv.EXPERIMENT_ID.value
else None
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could pack this into a method self._get_ert_meta() since it is used two places? Then it will be more aligned with the rest 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes absolutely, good point!

@jcrivenaes
Copy link
Collaborator

Ref PR description

it will be appended to the exported case metadata

I think it is correct to say it will be added to the fmu metadata block

Copy link
Collaborator

@jcrivenaes jcrivenaes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

I was a bit worried that taking a uuid.UUID(x), where x already is a uuid is needed or could give something else, but it seems to give the same.

@slangeveld slangeveld changed the title ENH: Add Ert experiment id to case metadata ENH: Add Ert experiment id FMU metadata block Aug 23, 2024
@slangeveld slangeveld changed the title ENH: Add Ert experiment id FMU metadata block ENH: Add Ert experiment id to FMU metadata block Aug 23, 2024
@slangeveld slangeveld merged commit fc19b07 into equinor:main Aug 26, 2024
13 checks passed
@slangeveld slangeveld deleted the 670-add-ert_experiment_id-to-case_metadata branch August 26, 2024 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include _ERT_EXPERIMENT_ID in case metadata
4 participants