TST: Remove deprecated patterns in tests to silence warnings #742

tnatt · 2024-07-05T12:18:44Z

Closes #735

Mainly fixed by:

adding config and content to ExportData instances that doesn't have it
replacing access_ssdl with classification and rep_include arguments
removing arguments from export/generate_metadata functions
removing vertical_domain as dictionary and adding new domain_reference argument
wrapping things in pytest.warns were appropiate

With these changes we are down to 4 warnings all coming from ERT regarding deprecated pydantic behaviour.

tnatt · 2024-07-05T12:20:31Z

src/fmu/dataio/dataio.py

@@ -378,7 +378,7 @@ class ExportData:
    classification: Optional[str] = None
    config: dict = field(default_factory=dict)
    content: Optional[Union[dict, str]] = None
-    depth_reference: str = "msl"  # deprecated
+    depth_reference: Optional[str] = None  # deprecated


This should have been done in #624 . Now it emits a deprecation warning every time ExportData is initialized 😱

tnatt · 2024-07-05T12:23:12Z

src/fmu/dataio/datastructure/configuration/global_configuration.py

-    rep_include: Optional[bool] = Field(
-        default=False,
-    )
+    rep_include: Optional[bool] = Field(default=None)


Changed the default here to None, to distinguish when it is present in the users config. And to avoid an incorrect UserWarning saying it should be removed from the config.
Should have been done in #694

Is there a breaking change possible from this out in the wild?

It should not be at the moment. The roundtrip we do on the config where we dump it back to dictionary while dropping None should then only trigger this 👇 part of the code if rep_include has an actual value in the config:

fmu-dataio/src/fmu/dataio/dataio.py

Line 525 in 827f293

if "rep_include" in self.config.get("access", {}).get("ssdl", {}):

But to be more future proof for when we address #629, I will change it to
if self.config.get("access", {}).get("ssdl", {}).get("rep_include") is not None:

Actually when we have the config as a pydantic object we can use the model_fields_set property to check this instead 🙂

tnatt · 2024-07-05T12:25:27Z

src/fmu/dataio/providers/_fmu.py


        logger.debug("No case metadata, issue a warning!")
-        warn("Case metadata does not exist, metadata will be empty!", UserWarning)
+        warn(


It was a bit many warnings emitted here when the case metadata was not present, so I merged the messages to one

tnatt · 2024-07-05T12:26:46Z

tests/test_units/test_contents.py

@@ -86,7 +86,7 @@ def test_content_fluid_contact_raises_on_invalid_contact(regsurf, globalconfig2)
        ExportData(
            config=globalconfig2,
            name="MyName",
-            content={"fluid_contact": {"contact": "OEC"}},
+            content={"fluid_contact": {"contact": "oec"}},


changed this to lowercase to avoid the UserWarning on top of the ValueError that the test is mainly checking.

I wonder if this warning (I added it) is misguided. I can't recall our discussion of it but should we support all upper-case for contacts too, since that is generally the convention? Maybe not the PR to discuss this on but came to mind

Now that you mention it.. I think we should, it is as you say a very common convention (more common than lowercase letters IMO). Then we can convert it to lowercase for them without emitting the message 😊

tnatt · 2024-07-05T12:29:10Z

tests/test_units/test_dataio.py

@@ -55,14 +44,15 @@ def test_missing_or_wrong_config_exports_with_warning(monkeypatch, tmp_path, reg

    monkeypatch.chdir(tmp_path)

-    with pytest.warns(UserWarning, match=pydantic_warning()):
+    with pytest.warns(UserWarning, match="The global config"):


Two UserWarnings are emitted everytime the config is invalid, this match string matches both of them.. hence the change.

tnatt · 2024-07-05T12:30:01Z

tests/test_units/test_dataio.py

@@ -437,6 +455,7 @@ def test_content_deprecated_seismic_offset(regsurf, globalconfig2):
    }


+@pytest.mark.filterwarnings("ignore: Number of maps nodes are 0")


warning coming from xtgeo, not relevant for this test

tnatt · 2024-07-05T12:31:10Z

tests/test_units/test_ert_context.py

@@ -45,6 +45,7 @@ def test_incl_jobs_warning(rmsglobalconfig):
            config=rmsglobalconfig,
            content="depth",
        )
+    dataio.ExportData.include_ertjobs = False


not adding a reset of the class variable here was triggering warnings in the tests below

mferrera

A large effort, nice job. This will make viewing test results so much more pleasant.

mferrera · 2024-07-05T12:39:01Z

src/fmu/dataio/aggregation.py

+        dummy_content = "depth"  # will not be used, but will silence warning
+        etemp = dataio.ExportData(config=config, name=self.name, content=dummy_content)


I don't have a big problem with this, but it does seem like something that becomes an oddity in future. Not sure on where something better might live (if there is something better)

I could not agree more, this hack is not pretty 😬 .. but this part of the code was already hacky, we initialize a dummy ExportData instance just to get to the ObjectDataProvider... so I figured I could sneak this in as well 😄

It is hopefully not going to live long into the future. This AggregatedData class is in need for some attention. Introducing pydantic but also refactoring at least this part of the code 👍

I was considering only silencing the warning in the tests, but the aggregation service in Sumo is also using this.. and this warning regarding missing content is just not relevant for the aggregation.

src/fmu/dataio/aggregation.py

mferrera · 2024-07-05T12:40:33Z

src/fmu/dataio/datastructure/configuration/global_configuration.py

-    rep_include: Optional[bool] = Field(
-        default=False,
-    )
+    rep_include: Optional[bool] = Field(default=None)


Is there a breaking change possible from this out in the wild?

mferrera · 2024-07-05T12:44:41Z

tests/test_units/test_contents.py

@@ -86,7 +86,7 @@ def test_content_fluid_contact_raises_on_invalid_contact(regsurf, globalconfig2)
        ExportData(
            config=globalconfig2,
            name="MyName",
-            content={"fluid_contact": {"contact": "OEC"}},
+            content={"fluid_contact": {"contact": "oec"}},


I wonder if this warning (I added it) is misguided. I can't recall our discussion of it but should we support all upper-case for contacts too, since that is generally the convention? Maybe not the PR to discuss this on but came to mind

jcrivenaes

Very nice; nothing to add what is already discussed

tnatt requested review from mferrera and jcrivenaes July 5, 2024 12:18

tnatt commented Jul 5, 2024

View reviewed changes

mferrera approved these changes Jul 5, 2024

View reviewed changes

tnatt force-pushed the test_warnings branch from 350e82e to 6d63d09 Compare July 6, 2024 07:15

mferrera assigned tnatt Jul 8, 2024

jcrivenaes approved these changes Jul 8, 2024

View reviewed changes

TST: Remove deprecated patterns in tests to silence warnings

aec6d96

tnatt force-pushed the test_warnings branch from 6d63d09 to aec6d96 Compare July 8, 2024 07:41

tnatt merged commit 14844cf into equinor:main Jul 8, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TST: Remove deprecated patterns in tests to silence warnings #742

TST: Remove deprecated patterns in tests to silence warnings #742

tnatt commented Jul 5, 2024

tnatt Jul 5, 2024

tnatt Jul 5, 2024

mferrera Jul 5, 2024

tnatt Jul 6, 2024

tnatt Jul 5, 2024

tnatt Jul 5, 2024

mferrera Jul 5, 2024

tnatt Jul 6, 2024 •

edited

Loading

tnatt Jul 5, 2024

tnatt Jul 5, 2024

tnatt Jul 5, 2024

mferrera left a comment

mferrera Jul 5, 2024

tnatt Jul 6, 2024

mferrera Jul 5, 2024

mferrera Jul 5, 2024

jcrivenaes left a comment

		@@ -437,6 +455,7 @@ def test_content_deprecated_seismic_offset(regsurf, globalconfig2):
		}


		@pytest.mark.filterwarnings("ignore: Number of maps nodes are 0")

		dummy_content = "depth" # will not be used, but will silence warning
		etemp = dataio.ExportData(config=config, name=self.name, content=dummy_content)

TST: Remove deprecated patterns in tests to silence warnings #742

TST: Remove deprecated patterns in tests to silence warnings #742

Conversation

tnatt commented Jul 5, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnatt Jul 6, 2024 • edited Loading

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

mferrera 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

Choose a reason for hiding this comment

jcrivenaes left a comment

Choose a reason for hiding this comment

tnatt Jul 6, 2024 •

edited

Loading