Pydantic Evaluations Conversion #388

christophertubbs · 2023-07-12T19:16:27Z

closes #261

Converts Evaluation Specification classes to Pydantic models.

christophertubbs · 2023-07-12T19:17:20Z

All that's really left is to ensure that everything works through the entire evaluation service pipeline.

aaraney

Great stuff, @christophertubbs! I left a few minor comments that should be pretty straight forward to get through. Thanks!

python/lib/core/dmod/core/common/helper_functions.py

python/lib/core/dmod/core/common/types.py

python/lib/evaluations/dmod/evaluations/specification/backend.py

aaraney · 2023-07-21T20:11:51Z

python/lib/evaluations/dmod/evaluations/specification/base.py

-        fields = {
-            "properties": self.properties.copy() if self.properties else dict()
-        }
+        if exclude_defaults is None:


why not just set the parameter default values to the default that you are assigning here?

It's a basic protection approach. I found one page describing some of it (here)[https://medium.com/python-features/how-to-avoid-classic-pitfall-while-passing-default-values-in-python-7002c0dc4c7c]. It's more important for objects, but it's a protection approach here because None is an invalid, but acceptable input. I can go ahead and remove it for primitive values if you'd like.

Yeah, default object types are a for sure no-no in python, but "copy" types in python are totally fair to use as defaults and I think make the code more clear. If you feel obliged to change it, go for it! But im not going to hold this up over something this small!

python/lib/evaluations/dmod/evaluations/specification/evaluation.py

python/lib/evaluations/dmod/evaluations/specification/fields.py

aaraney · 2023-07-24T13:27:49Z

python/lib/evaluations/dmod/evaluations/specification/fields.py

    def apply_configuration(
        self,
        configuration: typing.Dict[str, typing.Any],
        template_manager: TemplateManager,
        decoder_type: typing.Type[json.JSONDecoder] = None
    ):
-        self.__where = configuration.get("where", self.__where)
-        self.__datatype = configuration.get("datatype", self.__datatype)
+        self.where = configuration.get("where", self.where)


This feels like it could lead to unexpected behavior. I know if I were testing this, I would provide the wrong type just to see what happens. It feels like the most sane thing to do is to check if it exists and is a str then allow the mutation, otherwise throw. The same for datatype. Thoughts?

Suggested change

self.where = configuration.get("where", self.where)

if "where" in configuration:

if not isinstance(configuration["where"], str):

raise TypeError("value of key 'where' must be type str.")

self.where = configuration["where"]

Updated to mimic the provided logic for datatype, but changed the logic for where to make sure the given value matches the supplied literal values in the where definition

python/lib/evaluations/dmod/evaluations/specification/locations.py

aaraney · 2023-07-24T13:41:06Z

python/lib/evaluations/dmod/evaluations/specification/scoring.py

-    def __eq__(self, other: "SchemeSpecification") -> bool:
+    metric_functions: typing.List[MetricSpecification] = Field(
+        description="The metrics to perform within the evaluation",
+        alias="metrics"


Just pointing out, what can be a gotcha with pydantic < 2. alias field names do not propogate to subclasses unless you define them in Config.fields (i.e. fields = {"metric_functions": "metrics"}). I didnt see where this was being inherited from, so it is not an issue now, but just something to watch out for.

Updated. This might bite us soon since it looks like Config classes are being deprecated.

aaraney

Thanks for making the requested changes! I have a few comments that should be pretty easy to work through and then this can be on its way! Thanks again, @christophertubbs!

python/lib/evaluations/dmod/evaluations/data_retriever/disk.py

python/lib/evaluations/dmod/evaluations/specification/backend.py

python/lib/evaluations/dmod/evaluations/specification/fields.py

aaraney · 2023-08-04T15:35:07Z

python/lib/evaluations/dmod/evaluations/specification/locations.py

@@ -68,8 +68,34 @@ def apply_configuration(
        template_manager: TemplateManager,
        decoder_type: typing.Type[json.JSONDecoder] = None
    ):
-        self.from_field = configuration.get("from_field", self.from_field)
-        self.pattern = configuration.get("pattern", self.pattern)
+        if "from_field" in configuration:


Similar to my comment in the dmod.evaluations.specification.fields module, could this be simplified by creating a new instance of LocationSpecification from the configuration parameter and mutating self using the new instances parameters? Im probably missing something. I feel like is something you would have done if it were plausible here.

LocationSpecification is one of the few classes where this could work considering that all of its fields are optional (i.e. just about any combination is a-ok). What would that look like? Just something like:

location_template = self.__class__.parse_obj(configuration) if location_template.from_field is not None: # assign # etc

?

location_template = self.__class__.parse_obj(configuration) if location_template.from_field is not None: # assign # etc

you could even just do this:

location_template = type(self).parse_obj(configuration)

aaraney

Thanks for making the requested changes, @christophertubbs! Merge at will!

…' to 'backend_type' for Backend specifications and converted evaluation specification objects to Pydantic

… TypeDefinition.matches that used the old `complies` nomenclature.

…tion` (it's now just `backend`).

…ementations.

…o read and changed the type in the ValueSelector to be one of several optional string values rather than ANY string

…ere' and 'datatype' in a specification configuration

…ld be case insensitive

…eld` and updated the overriding logic for `from_field` and `pattern` in `LocationSpecification` to ensure that bytes are sufficiently decoded and string types are enforced.

…s` to allow pre-2.0 behavior.

…nd added a TODO to the disk data_retriever to inform that the 'date_parser' logic needs to be replaced

…e specification before trying to reference it

christophertubbs added the enhancement New feature or request label Jul 12, 2023

christophertubbs marked this pull request as ready for review July 13, 2023 20:01

christophertubbs requested a review from aaraney July 13, 2023 20:01

aaraney requested changes Jul 24, 2023

View reviewed changes

aaraney requested changes Aug 4, 2023

View reviewed changes

aaraney previously approved these changes Aug 4, 2023

View reviewed changes

christophertubbs added 16 commits August 4, 2023 15:37

Changed the configuration members 'data_format' to 'format' and 'type…

f01973c

…' to 'backend_type' for Backend specifications and converted evaluation specification objects to Pydantic

Updated the version numbers for dmod.core and dmod.evaluations

bcd4d22

Removed an unused function in TypeDefinition and fixed a reference to…

380db1d

… TypeDefinition.matches that used the old `complies` nomenclature.

Provided two module descriptions

eb715f7

Fixed an outdated reference to _backend within the `LoaderSpecifica…

0dc0ec4

…tion` (it's now just `backend`).

Added docstrings and explanation for why abstract functions have impl…

34dab27

…ementations.

Removed an unnecessary reassignment of Specification.name

0dc582f

Fixed naming issues within the base specification becomes confusing t…

8495686

…o read and changed the type in the ValueSelector to be one of several optional string values rather than ANY string

Applied suggested changes to help ensure safetype when overriding 'wh…

90dad4e

…ere' and 'datatype' in a specification configuration

Updated the overriding of where in a ValueSelector so that it wou…

1bd69b2

…ld be case insensitive

Added some value check protections for `LocationSpecification.from_fi…

681b6c9

…eld` and updated the overriding logic for `from_field` and `pattern` in `LocationSpecification` to ensure that bytes are sufficiently decoded and string types are enforced.

Added some value check protections for `LocationSpecification.from_fi…

c5ae42f

…eld` and updated the overriding logic for `from_field` and `pattern` in `LocationSpecification` to ensure that bytes are sufficiently decoded and string types are enforced.

Updated the alias definition for `SchemaSpecification.metric_function…

2cba55b

…s` to allow pre-2.0 behavior.

Fixed unit tests to enforce proper 'where' keys for value selectors a…

0a80132

…nd added a TODO to the disk data_retriever to inform that the 'date_parser' logic needs to be replaced

Added checks to see if a locations specification is on a data sourc…

aa26f0f

…e specification before trying to reference it

Added dynamic NextJS artifacts to the .gitignore

e7abba4

christophertubbs dismissed aaraney’s stale review via e7abba4 August 4, 2023 20:38

christophertubbs force-pushed the evaluation_spec_to_pydantic branch from ee563a9 to e7abba4 Compare August 4, 2023 20:38

christophertubbs requested a review from aaraney August 7, 2023 12:55

aaraney approved these changes Aug 7, 2023

View reviewed changes

christophertubbs merged commit ec17507 into NOAA-OWP:master Aug 7, 2023
1 check passed

christophertubbs deleted the evaluation_spec_to_pydantic branch October 19, 2023 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pydantic Evaluations Conversion #388

Pydantic Evaluations Conversion #388

christophertubbs commented Jul 12, 2023

christophertubbs commented Jul 12, 2023

aaraney left a comment

aaraney Jul 21, 2023

christophertubbs Jul 25, 2023

aaraney Aug 4, 2023

aaraney Jul 24, 2023

aaraney Jul 24, 2023

christophertubbs Jul 25, 2023

aaraney Jul 24, 2023

christophertubbs Jul 25, 2023

aaraney left a comment

aaraney Aug 4, 2023

christophertubbs Aug 4, 2023

aaraney Aug 4, 2023

aaraney left a comment

-        self.where = configuration.get("where", self.where)
+        if "where" in configuration:
+            if not isinstance(configuration["where"], str):
+                raise TypeError("value of key 'where' must be type str.")
+            self.where = configuration["where"]

Pydantic Evaluations Conversion #388

Pydantic Evaluations Conversion #388

Conversation

christophertubbs commented Jul 12, 2023

christophertubbs commented Jul 12, 2023

aaraney 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

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

aaraney 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

aaraney left a comment

Choose a reason for hiding this comment