-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: Pydantic model for the serialization schema #888
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #888 +/- ##
==========================================
- Coverage 85.55% 85.50% -0.06%
==========================================
Files 78 79 +1
Lines 14397 14416 +19
Branches 14397 14397
==========================================
+ Hits 12318 12327 +9
- Misses 1445 1455 +10
Partials 634 634
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Sorry for the noisy metadata changes, the CI still needed some fixes to work correctly... |
So mypy stops complaining
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.
Nice, the Python stuff looks good to me 👍 Can't comment on the CI though :D
Now, what would be the best path for Guppy to migrate to this? Publish quantinuum_hugr
on PyPI or pull in the dependency from this repo for now?
vs: list["Const"] | ||
|
||
class Config: | ||
# Need to avoid random '\n's in the pydantic description |
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 is quite annoying :(
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.
Yep :/
class Config: | ||
# Need to avoid random '\n's in the pydantic description | ||
json_schema_extra = { | ||
"description": "A Sum variant For any Sum type where this value meets the type of the variant indicated by the tag.", |
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.
"description": "A Sum variant For any Sum type where this value meets the type of the variant indicated by the tag.", | |
"description": "A Sum variant. For any Sum type where this value meets the type of the variant indicated by the tag.", |
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.
Also, why is ruff not complaining about the long line?
It might be nicer to split the string into multiple lines
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.
Done 👍
# The types will be all None because it's not dataflow, but we only care about | ||
# the number of outputs. Note that we don't make use of the HUGR feature where | ||
# the variant data is appended to successor input. Thus, `predicate_variants` | ||
# will only contain empty rows. |
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.
Should we get rid of these Guppy specific comments?
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.
👍
We could publish a I would wait for the builder API before releasing |
This PR ports the pydantic definition from guppy (including CQCL/guppylang#172), and updates it to sync up with the current schema.
Changes to the schema:
InputExtensions
class toExtensionSet
Const
DummyOp
. It was referenced in a couple field types, but the model was never defined.The main difference with the model in
guppy
is the use ofRootModel
-based classes instead of variables assigned to aTypeAliasType
for defining aliases on type unions.They both work the same, but
mypy
doesn't like using variables in types.Additionally, I updated some READMEs and added a CI check to ensure that the json in
specification/schema/
is always up to date.