feat(hugr-py): Define typed Metadata protocol #2765
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2765 +/- ##
==========================================
+ Coverage 83.52% 83.71% +0.18%
==========================================
Files 268 267 -1
Lines 53162 53145 -17
Branches 47871 47730 -141
==========================================
+ Hits 44405 44488 +83
+ Misses 6371 6276 -95
+ Partials 2386 2381 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
aborgna-q
force-pushed
the
ab/py-metadata
branch
from
7460a59 to
2f2572b
Compare
aborgna-q
force-pushed
the
ab/py-metadata
branch
from
2f2572b to
e6d329a
Compare
maximilianruesch
left a comment
maximilianruesch
left a comment
There was a problem hiding this comment.
Thanks, this will be a very useful addition! I have a bunch of suggested changes, some more nitty, some questions, some remarks for later.
| @classmethod | ||
| def to_json(cls, value: GeneratorDesc) -> dict[str, str]: | ||
| return value._to_json() | ||
|
|
||
| @classmethod | ||
| def from_json(cls, value: Any) -> GeneratorDesc: | ||
| return GeneratorDesc._from_json(value) |
There was a problem hiding this comment.
If we have a lot of "simple" metadata keys that do not require special validation (only calling the JSON serialize / deserialize of their wrapped type), we might want to think about creating something like a mixin for such cases to reduce boilerplate / verbosity. I do think we need a bit more data / keys to assess what the best solution for this is, so there is no need to refactor this right now.
There was a problem hiding this comment.
👍
For native-type metadata (ints, strings, lists, and dicts) there's no need to override these methods. But yeah, it'd be nice to have a deserialize/_serialize`
I guess we could use pydantic for that? It's a bit annoying that we're starting from the already-loaded object rather than a json string.
There was a problem hiding this comment.
I have not yet used pydantic, but it strikes me as very powerful. If we adopt it, we should assess whether there is considerable overhead to serialization / deserialization that is not ignorable in the context of hugr-py.
| @classmethod | ||
| def to_json(cls, value: list[ExtensionDesc]) -> list[dict[str, str]]: | ||
| return [e._to_json() for e in value] | ||
|
|
||
| @classmethod | ||
| def from_json(cls, value: Any) -> list[ExtensionDesc]: | ||
| if not isinstance(value, list): | ||
| msg = ( | ||
| "Expected UsedExtensions metadata to be a list," | ||
| + f" but got {type(value)}" | ||
| ) | ||
| raise TypeError(msg) | ||
| return [ExtensionDesc._from_json(e) for e in value] |
There was a problem hiding this comment.
Similar to the mixin suggestion, lists of wrapped types may be another example of a common pattern that could occur multiple times.
| name: str | ||
| version: Version | None | ||
|
|
||
| def _to_json(self) -> dict[str, str]: |
There was a problem hiding this comment.
Is it useful to have a protocol for _to_json and _from_json? Also, should these be non-private since they are called from outside the class?
There was a problem hiding this comment.
I wanted to keep the PR simple and focused on the metadata protocol for now.
We should probably look into serializable protocols for the value types. (does pydantic help here?), but I'll leave it for later.
The _to/_from_json shouldn't normally be called by users. I'd love to have a pub(crate) attribute here...
There was a problem hiding this comment.
While its true that a user has no benefit to calling these methods, there is no harm either, since its either read-only or a constructing method, i.e. they cannot violate any contracts with using these methods. I am unsure as to what the policy of this repo is, i.e. whether a cleaner user interface or adhering to general Python guidelines has a higher priority.
maximilianruesch
left a comment
maximilianruesch
left a comment
There was a problem hiding this comment.
A preliminary LGTM on the code, save for the last comment on _to_json visibility, and some failing coverage checks (which imo one should address in some way, since the code contains many paths).
Adds a
Metadataprotocol similar to the one in the rust side, and exports the rust key definitions.Depends on #2764