refactor(clp-package): Use annoated serializer to avoid custom serialization. #1417
Conversation
…s and update serialization.
…t directly to model_dump.
…drop custom port validators.
…ost type and removing duplicated validators.
…type and adjust serialization accordingly.
…red NonEmptyStr type.
…n scheduler configs.
…heduler configs and update references.
…ndant validators.
…remove its validator.
…el type and remove validation helpers.
…ng about plan for replacement.
WalkthroughRefactors config models to use string-serialized enum aliases and path-as-string types (PathStr) via new Pydantic serializers. Removes multiple dump_to_primitive_dict methods, adds a pre-validation hook in AwsAuthentication, and updates public field annotations and serialization behavior across config-related models. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Config as CLPConfig / Package / Models
participant Auth as AwsAuthentication
participant Pyd as Pydantic (validators)
participant Ser as Serializers (StrEnumSerializer / PathStr)
User->>Config: Instantiate models with enums and paths
activate Pyd
Pyd->>Auth: Run model validators
note right of Auth #DDEBF7: validate_authentication (mode="before")
Auth-->>Pyd: Normalized auth data
deactivate Pyd
User->>Config: Serialize models (e.g., .model_dump())
activate Ser
Ser-->>Config: Enums -> string via .value
Ser-->>Config: Paths -> string via str(path)
deactivate Ser
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/clp-py-utils/clp_py_utils/clp_config.py (1)
672-672: Fix broken enum comparison after type change.After changing
AwsAuthentication.typetoAwsAuthTypeStr(an enum with string serialization), the comparison at Line 672 is now broken. The code comparesAwsAuthType.profile.value(a string) withauth.type(anAwsAuthTypeenum object), which will always evaluate toFalse. This breaks the validation logic that checks whether profile authentication is used.Apply this diff to fix the comparison:
- if AwsAuthType.profile.value == auth.type: + if AwsAuthType.profile == auth.type:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/clp-py-utils/clp_py_utils/clp_config.py(9 hunks)components/clp-py-utils/clp_py_utils/pydantic_serialization_utils.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: package-image
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: rust-checks (ubuntu-22.04)
- GitHub Check: rust-checks (macos-15)
- GitHub Check: rust-checks (ubuntu-24.04)
🔇 Additional comments (8)
components/clp-py-utils/clp_py_utils/pydantic_serialization_utils.py (2)
6-6: LGTM!The
StrEnumSerializercorrectly extracts the string value from enum instances for serialization, enabling clean enum-to-string conversion in Pydantic models.
8-8: LGTM!The
PathStrtype correctly serializespathlib.Pathobjects as strings, eliminating the need for custom serialization methods in models.components/clp-py-utils/clp_py_utils/clp_config.py (6)
24-24: LGTM!The import correctly brings in the new serialization utilities that enable cleaner, more declarative model serialization.
118-118: LGTM!The string-serialized enum type aliases correctly wrap the base enum types with
StrEnumSerializer, ensuring enums are serialized as their string values rather than enum objects.Also applies to: 126-126, 135-135, 150-150
154-155: LGTM!The field type updates correctly use the string-serialized enum aliases, maintaining enum type safety at runtime while enabling automatic string serialization.
Also applies to: 181-181, 369-369
373-398: LGTM!The pre-validation hook correctly validates the authentication configuration, ensuring that required fields are set based on the authentication type and preventing invalid combinations.
415-415: LGTM!The path fields correctly use
PathStrto enable automatic string serialization, eliminating the need for custom serialization methods.Also applies to: 430-430, 453-453, 457-457, 461-461, 465-465, 469-469
571-571: LGTM!The
CLPConfigpath fields correctly usePathStrfor automatic string serialization, including the private attributes.Also applies to: 577-582
| import pathlib | ||
| from typing import Annotated | ||
|
|
||
| from pydantic import PlainSerializer | ||
|
|
||
| StrEnumSerializer = PlainSerializer(lambda enum_value: enum_value.value) | ||
|
|
||
| PathStr = Annotated[pathlib.Path, PlainSerializer(lambda path_value: str(path_value))] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding docstrings for public utilities.
Adding a module docstring and docstrings for StrEnumSerializer and PathStr would improve discoverability and maintainability. Consider also adding an __all__ export list to explicitly declare the public API.
Apply this diff to add documentation:
+"""
+Pydantic serialization utilities for common types.
+
+Provides annotated types and serializers to handle enum and path serialization
+without requiring custom model serialization methods.
+"""
+
import pathlib
from typing import Annotated
from pydantic import PlainSerializer
+# Serializer that converts enum instances to their string values
StrEnumSerializer = PlainSerializer(lambda enum_value: enum_value.value)
+# Annotated type for pathlib.Path that serializes to string
PathStr = Annotated[pathlib.Path, PlainSerializer(lambda path_value: str(path_value))]
+
+__all__ = ["StrEnumSerializer", "PathStr"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import pathlib | |
| from typing import Annotated | |
| from pydantic import PlainSerializer | |
| StrEnumSerializer = PlainSerializer(lambda enum_value: enum_value.value) | |
| PathStr = Annotated[pathlib.Path, PlainSerializer(lambda path_value: str(path_value))] | |
| """ | |
| Pydantic serialization utilities for common types. | |
| Provides annotated types and serializers to handle enum and path serialization | |
| without requiring custom model serialization methods. | |
| """ | |
| import pathlib | |
| from typing import Annotated | |
| from pydantic import PlainSerializer | |
| # Serializer that converts enum instances to their string values | |
| StrEnumSerializer = PlainSerializer(lambda enum_value: enum_value.value) | |
| # Annotated type for pathlib.Path that serializes to string | |
| PathStr = Annotated[pathlib.Path, PlainSerializer(lambda path_value: str(path_value))] | |
| __all__ = ["StrEnumSerializer", "PathStr"] |
🤖 Prompt for AI Agents
In components/clp-py-utils/clp_py_utils/pydantic_serialization_utils.py around
lines 1 to 8, add a module-level docstring describing the purpose (helpers to
create Pydantic PlainSerializers for common types), add concise
docstrings/comments for the StrEnumSerializer and PathStr explaining what they
serialize and expected input/output (e.g., StrEnumSerializer converts Enum
instances to their .value string; PathStr serializes pathlib.Path to str), and
add an __all__ list exporting ("StrEnumSerializer", "PathStr") so the public API
is explicit; keep wording short and place docstrings immediately above the
definitions.
There was a problem hiding this comment.
For the file name, how about just file_utils? Though I understand the utils here likely are only compatible with Pydantic, the name "Pydantic" is still a bit implementation-specific which i feel can be omitted from the name.
There was a problem hiding this comment.
actually, can we also:
- Only export the lambda functions in
serialization_utils.py:serialize_path = lambda ... - Create the Pydantic types in
clp_config.py(or move them into a separatetypes.pysource file. maybe in a future refactoring PR) e.g.,
# in clp_config.py
PathStr = Annotated[
pathlib.Path,
PlainSerializer(serialize_path)
]so that we can decouples the serialization utils from Pydantic?
There was a problem hiding this comment.
I feel TypeStr is not a very intuitive name. Is there a better suggestion?
| _container_image_id_path: PathStr = PrivateAttr(default=CLP_PACKAGE_CONTAINER_IMAGE_ID_PATH) | ||
| _version_file_path: PathStr = PrivateAttr(default=CLP_VERSION_FILE_PATH) |
There was a problem hiding this comment.
(just to show we have considered -)
Since these are PrivateAttr, they won't serialize by default anyway. The type change is harmless though.
|
|
||
| StrEnumSerializer = PlainSerializer(lambda enum_value: enum_value.value) | ||
|
|
||
| PathStr = Annotated[pathlib.Path, PlainSerializer(lambda path_value: str(path_value))] |
There was a problem hiding this comment.
(i'm not sure if this is worth doing but we shall investigate)
can we embed the validator _validate_directory with BeforeValidator() into the Annotation as well?
https://docs.pydantic.dev/latest/api/functional_validators/#pydantic.functional_validators.BeforeValidator
sitaowang1998
changed the title
2 participants
Description
This PR adds pydantic annotated serializer for
pathlib.PathandStrEnumto avoid adding custom model serialization for fields of these types.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Refactor