Refactor evolver.base #217

jamienoss · 2024-11-23T21:35:39Z

#213 Was the main driver for this refactor. However, it does not fix that issue but instead refactors some code to make things more manageable to future changes.

The core change is the creation of _BaseModel for the separation of conversion back and forth between Config, instances, and ConfigDescriptors such that it is clearer where in the code this happens - it is no longer circular. For clarity _BaseModel does not handle the conversion between Config and configDescriptor and instead only includes other base conversions, i.e., json strings. Soln. to #234 would also go in this new class. This PR introduces some duplication in logic, but at the expense of it being more direct and explicit per specific desired class behavior.

This should also help and relate to #234.

codecov · 2024-11-23T21:36:53Z

Codecov Report

Attention: Patch coverage is 97.33333% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
evolver/base.py	97.14%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

Signed-off-by: James Noss <jnoss@jhu.edu>

jamienoss · 2025-01-08T16:57:47Z

evolver/base.py

-    @classmethod
-    def load(cls, file_path: Path, encoding: str | None = None):
-        """Loads the specified config file and return a new instance."""
-        with open(file_path, encoding=encoding) as f:
-            return cls.model_validate(yaml.safe_load(f))
-
-    def save(self, file_path: Path, encoding: str | None = None):
-        """Write out config as yaml file to specified file."""
-        file_path = Path(file_path)
-        file_path.parent.mkdir(parents=True, exist_ok=True)
-        with open(file_path, "w", encoding=encoding) as f:
-            yaml.dump(self.model_dump(mode="json"), f)
-


These were unnecessarily duplicated.

Signed-off-by: James Noss <jnoss@jhu.edu>

jamienoss · 2025-01-09T14:44:27Z

evolver/base.py

+            try:
+                # json str might be that for a ConfigDescriptor, so try that 1st.
+                descriptor = ConfigDescriptor.model_validate_json(obj, strict=strict, context=context)
+            except Exception:
+                # Ok, maybe it wasn't a ConfigDescriptor... (or was but was malformed)
+                # Explicitly call ``model_validate_json`` rather than ``super().model_validate()`` to declare explicit
+                # code logic.
+                return cls.model_validate_json(obj, strict=strict, context=context)
+            else:
+                # Cool, it was a ConfigDescriptor so validate from that. Remember ``ConfigDescriptor.config`` is just a
+                # dict.
+                return super().model_validate(
+                    descriptor.config, strict=strict, from_attributes=from_attributes, context=context
+                )


@amitschang It would be handy for #234 if this duplication got nuked. Does it make sense for us to lose info by converting from a descriptor to a naked Config?

It appears that there's currently no test in the suite for this (which I can add), however, I can't think of an existing or needed use case for this, can you? If not, maybe we should nuke it.

Test added.

Do you mean a need for converting from a ConfigDescriptor to Config (of the class specified in classinfo, I guess)? If the loss you are talking about is when serializing that Config object we lose the classinfo, then yeah I can't think of a reason we desire this.

But not sure if that is what you are asking.

Yeah, that's exactly it.

@jamienoss, is there something you will change here, then? Or is it ready?

Not in this PR as it opens questions to removing other sections of that switch also, i.e., all other sections dealing with descriptors, not just those as strings.

Sorry for the confusion, I just wanted to know as an aid to working on #234.

Opened #242 to address this.

Signed-off-by: James Noss <jnoss@jhu.edu>

amitschang

Looks good overall, just a couple comments

amitschang · 2025-01-10T15:43:47Z

evolver/base.py

@@ -8,6 +8,7 @@
 import pydantic
 import pydantic_core
 import yaml
+from pydantic._internal._validators import import_string


Should we be using this? the _internal and lack of docs make me think not. It looks like this is used to import from fqcn to a type, perhaps we could either make a util using importlib or a simple wrapper around a pydantic model that has ImportString type?

Looking at https://github.com/pydantic/pydantic/blob/main/pydantic/_internal/_validators.py#L72, it doesn't seem worth it to replicate this. Your second suggestion sound great though and will implement that, thanks!

amitschang · 2025-01-10T15:49:54Z

evolver/base.py

+                validate_classinfo(descriptor.classinfo)
+                config = descriptor.config
+
+        config_dict = config.shallow_model_dump() if isinstance(config, _BaseConfig) else config


It might be worth a note here for posterity as to why this is needed - is it because this could be a dict due to 287 or 301? In the latter case, should it be validated or has it already been?

Comment added. Also, note that this should be checked against _BaseModel and not _BaseConfig.

...and it will have been validated by then.

Signed-off-by: James Noss <jnoss@jhu.edu>

jamienoss · 2025-01-10T18:04:59Z

evolver/base.py

-            # Next handle dict | str, which could be that representing a Config or a ConfigDescriptor.
-            # First see if config is actually a descriptor by trying to create one, since it's only fields are limited
-            # to classinfo and config.
+            # Last is a str representing either a Config or ConfigDescriptor.
            try:
                descriptor = ConfigDescriptor.model_validate(config, context=dict(extra="forbid"))


Re #217 (comment), validation takes place here.

jamienoss · 2025-01-10T18:05:18Z

evolver/base.py

+        elif _is_descriptor_dict(config):
+            validate_classinfo(config["classinfo"])
+            config = cls.Config.model_validate(config["config"])
+        elif isinstance(config, dict):


Re #217 (comment), validation takes place here.

jamienoss added the refactor label Nov 23, 2024

jamienoss self-assigned this Nov 23, 2024

jamienoss added 3 commits January 8, 2025 11:30

Add serializer to convert to BaseInterface.Config -> ConfigDescriptor

51b0f46

Signed-off-by: James Noss <jnoss@jhu.edu>

Refactor evolver.base to better stream-line behavoir

72800de

Signed-off-by: James Noss <jnoss@jhu.edu>

all tests pass

ecfda50

Signed-off-by: James Noss <jnoss@jhu.edu>

jamienoss force-pushed the 213-fix-nested-configdescriptor-creation branch from 7d14cb2 to ecfda50 Compare January 8, 2025 16:32

jamienoss commented Jan 8, 2025

View reviewed changes

jamienoss force-pushed the 213-fix-nested-configdescriptor-creation branch from e5ca24c to ab64d08 Compare January 8, 2025 19:39

Self review changes

818accc

Signed-off-by: James Noss <jnoss@jhu.edu>

jamienoss force-pushed the 213-fix-nested-configdescriptor-creation branch from ab64d08 to 818accc Compare January 8, 2025 19:45

jamienoss marked this pull request as ready for review January 8, 2025 19:47

jamienoss requested a review from a team as a code owner January 8, 2025 19:47

jamienoss requested a review from amitschang January 8, 2025 19:47

jamienoss commented Jan 9, 2025

View reviewed changes

Add missing test

efa2172

Signed-off-by: James Noss <jnoss@jhu.edu>

jamienoss mentioned this pull request Jan 9, 2025

Don't convert from ConfigDescriptors to Config. #242

Open

amitschang reviewed Jan 10, 2025

View reviewed changes

jamienoss added 2 commits January 10, 2025 12:25

Emulate pydantic.import_string in evolver.util

75166ee

Signed-off-by: James Noss <jnoss@jhu.edu>

Post review changes

8163b5e

Signed-off-by: James Noss <jnoss@jhu.edu>

jamienoss requested a review from amitschang January 10, 2025 17:57

jamienoss enabled auto-merge (squash) January 10, 2025 17:58

jamienoss commented Jan 10, 2025

View reviewed changes

amitschang approved these changes Jan 10, 2025

View reviewed changes

jamienoss merged commit 600f130 into main Jan 10, 2025
8 checks passed

jamienoss deleted the 213-fix-nested-configdescriptor-creation branch January 10, 2025 18:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor evolver.base #217

Refactor evolver.base #217

jamienoss commented Nov 23, 2024 •

edited

Loading

codecov bot commented Nov 23, 2024 •

edited

Loading

jamienoss Jan 8, 2025

jamienoss Jan 9, 2025 •

edited

Loading

jamienoss Jan 9, 2025

amitschang Jan 9, 2025

jamienoss Jan 9, 2025

amitschang Jan 9, 2025

jamienoss Jan 9, 2025

jamienoss Jan 9, 2025

amitschang left a comment

amitschang Jan 10, 2025

jamienoss Jan 10, 2025

amitschang Jan 10, 2025

jamienoss Jan 10, 2025

jamienoss Jan 10, 2025

jamienoss Jan 10, 2025

jamienoss Jan 10, 2025

Refactor evolver.base #217

Refactor evolver.base #217

Conversation

jamienoss commented Nov 23, 2024 • edited Loading

codecov bot commented Nov 23, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

jamienoss Jan 9, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amitschang 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

jamienoss commented Nov 23, 2024 •

edited

Loading

codecov bot commented Nov 23, 2024 •

edited

Loading

jamienoss Jan 9, 2025 •

edited

Loading