equinor / fmu-dataio

FMU data standard and data export with rich metadata in the FMU context
https://fmu-dataio.readthedocs.io/en/latest/
Apache License 2.0
10 stars 15 forks source link

Evaluate need for continuation of deprecation for seismic and property contents without extra info #598

Closed tnatt closed 4 months ago

tnatt commented 6 months ago

seismic and propertycontents without extra info is deprecated, but it is required in the schema. After changing to early validation of metadata https://github.com/equinor/fmu-dataio/pull/550, a deprecation warning will be printed but it will not pass validation in generate_metadata()

It does not make sense to try to make this old pattern valid internally, when the metadata produced will not be considered valid for sumo anyway. Can we remove the deprecation?

I am not familiar with the history here, but there exists references to this being kept for backwards compatibility.

https://github.com/equinor/fmu-dataio/blob/6ba4b4903da1c73f3d1a82154df50a727388b540/src/fmu/dataio/datastructure/_internal/internal.py#L120-L135

tnatt commented 6 months ago

@perolavsvendsen @jcrivenaes Do you have any thoughts here?

perolavsvendsen commented 6 months ago

Are the example scripts in Drogon doing this correctly? If that is the case, I am inclined to say that we could discontinue it... I am not really familiar with the history, either 🤷‍♂️

jcrivenaes commented 6 months ago

I think it can be discontinued 👍

tnatt commented 6 months ago

Update - actually turns out it is only content="seismic" that requires extra info inside a data.seismic block when it comes to the schema. content="property" is not defined with any extra fields in the schema hence a data.property is not required

property: https://github.com/equinor/fmu-dataio/blob/73001ef2d7bbb88a324861f133f8ebc94e6225fc/src/fmu/dataio/datastructure/meta/content.py#L287-L288

seismic: https://github.com/equinor/fmu-dataio/blob/73001ef2d7bbb88a324861f133f8ebc94e6225fc/src/fmu/dataio/datastructure/meta/content.py#L307-L311

We need to decide on what to do for content="property"

tnatt commented 5 months ago

Ref offline discussions with @perolavsvendsen and @jcrivenaes 07.05 regarding what to do with content='property' and the fact that we are currently warning people to use a dictionary form with extra info (attribute and is_discrete) but any extra info will be dropped since a data.property is not defined in the schema.

Talking points:

We acknoledge that it is not easy to land on something that will not be subject to change for the user in the future. Probably the entire datamodel needs to be subject for review, and a data.product with dedicated API-endpoints connected to it is the way to go, but this is outside of scope at the moment and a much bigger task to address.

The two most likely ways forward as we see it:

  1. Remove the warning, and do no further changes. This allows us to let the datamodel mature a bit more until we can decide what to do with this content='property'. Downsides:

    • we don't start providing the attribute to our consumers yet
    • if we choose to do something different then what we have been warning about up until now, the users that actually followed the initial warning will be punished.
  2. Add data.property to the schema, with attribute and is_discrete fields, and keep the deprecation period for content='property' with no extra info.

    • decide on wether we should create a withelisted attribute list, or have it as freetext in the beginning until we have landed on what we want the users to do for e.g. a surface with facies_thickness (ref above).
tnatt commented 4 months ago

Closing this and created separate issue for content="property" #734