-
Notifications
You must be signed in to change notification settings - Fork 3.1k
[Core] Add Serializable protocol #43207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This is a protocol that custom models can implement to allow it to work with the SDK serialization/deserialization mechanisms. Signed-off-by: Paul Van Eck <[email protected]>
) | ||
""" | ||
|
||
def to_dict(self) -> Dict[str, Any]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we expecting people to implement any internal recursion themselves? Or do we do that for them?
I was wondering if instead of using Dict[str, Any]
, whether we want to lock the value types down to the types that we know our JSON encoder supports....
Not sure how feasible and/or useful that would be....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SdkJSONEncoder does automatically handle nested custom types and will call the nested type to_dict methods internally. E.g.: if someone implements to_dict like:
Class FooModel:
foo: str
bar: BarModel
...
def to_dict(self) -> Dict[str, Any]:
return {"foo": self.foo, "bar": self.bar}
bar.to_dict()
will be called internally during serialization.
For as_dict()
, we'd need some slight adjustments in the code. This would probably just be wrapping the output of to_dict with another _serialize call in the model_base.py file
Regarding the typing, I think keeping it simpler with Any
would be more flexible, especially if we are allowing output like the above with arbitrary types being values. The Union would be pretty large, and it's somewhat of a maintenence burden if our JSON encoder adds support for another type, as we'd have to update the Union.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new Serializable
protocol to Azure Core that allows custom models to integrate with the SDK's serialization and deserialization mechanisms. The protocol defines standard to_dict()
and from_dict()
methods that custom types can implement to control their JSON representation.
- Adds a runtime-checkable
Serializable
protocol withto_dict
andfrom_dict
methods - Updates the JSON encoder to handle objects implementing the
Serializable
protocol - Extends deserialization logic to work with custom serializable models
- Provides comprehensive test coverage for both standalone and embedded custom models
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
azure/core/serialization.py | Defines the new Serializable protocol with documentation and type annotations |
modeltypes/_utils/model_base.py | Updates SdkJSONEncoder and deserialization logic to handle Serializable objects |
tests/test_serialization.py | Adds comprehensive test suite for the new protocol functionality |
from datetime import timezone | ||
|
||
|
||
__all__ = ["NULL", "AzureJSONEncoder", "is_generated_model", "as_attribute_dict", "attribute_list"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Serializable
protocol should be added to __all__
to make it part of the public API since it's intended for external use by custom models.
__all__ = ["NULL", "AzureJSONEncoder", "is_generated_model", "as_attribute_dict", "attribute_list"] | |
__all__ = ["NULL", "AzureJSONEncoder", "is_generated_model", "as_attribute_dict", "attribute_list", "Serializable"] |
Copilot uses AI. Check for mistakes.
"bar": 42, | ||
"baz": 3.14, | ||
} | ||
json_dict = {"foo": "foo", "bar": 42, "baz": 3.14} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line duplicates the dictionary definition from line 1055-1059. Remove this duplicate assignment.
json_dict = {"foo": "foo", "bar": 42, "baz": 3.14} |
Copilot uses AI. Check for mistakes.
if isinstance(annotation, Serializable): | ||
return annotation.from_dict # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is incorrect. annotation
is a type/class, not an instance. Use issubclass(annotation, Serializable)
instead of isinstance(annotation, Serializable)
.
if isinstance(annotation, Serializable): | |
return annotation.from_dict # type: ignore | |
try: | |
if issubclass(annotation, Serializable): | |
return annotation.from_dict # type: ignore | |
except TypeError: | |
pass |
Copilot uses AI. Check for mistakes.
This is a protocol that custom models can implement to allow it to work with the SDK serialization/deserialization mechanisms.
Ref: https://gist.github.com/pvaneck/3f7266c07828d0fb3bd15e8cfbc618d5#2-protocol-based-approach
Open questions: