feat!: Serialised extensions #1371

ss2165 · 2024-07-26T14:46:14Z

Fear not the diff, mostly schema update.

Define a Pydantic model and serialised schema for extensions.
Update the rust SignatureFunc serialisation to be compatible with this.
Serialized extension "declarations" can state they require binary compute or validation functions. SignatureFunc reports this if they are missing, and it is up to the caller (typically validation) to decide how to recover from this.

Closes #1360
Closes #1361

Addresses parts of #1228

Best reviewed as individual commits.

BREAKING CHANGE: TypeDefBound uses struct-variants for serialization. SignatureFunc now has variants for missing binary functions, and serializes in to a new format that indicates expected binaries.

codecov · 2024-07-26T14:50:44Z

Codecov Report

Attention: Patch coverage is 83.13253% with 42 lines in your changes missing coverage. Please review.

Project coverage is 87.67%. Comparing base (b7a39b9) to head (9990e78).
Report is 1 commits behind head on main.

Files	Patch %	Lines
...e/src/extension/op_def/serialize_signature_func.rs	79.66%	19 Missing and 5 partials ⚠️
hugr-core/src/extension/op_def.rs	50.00%	16 Missing and 1 partial ⚠️
hugr-py/src/hugr/serialization/extension.py	97.72%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1371      +/-   ##
==========================================
- Coverage   87.71%   87.67%   -0.05%     
==========================================
  Files         113      116       +3     
  Lines       20081    20270     +189     
  Branches    17815    17969     +154     
==========================================
+ Hits        17615    17771     +156     
- Misses       1691     1717      +26     
- Partials      775      782       +7

Flag	Coverage Δ
python	`91.61% <97.82%> (+0.08%)`	⬆️
rust	`87.16% <79.80%> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

acl-cqc · 2024-07-29T11:19:35Z

hugr-core/src/extension/op_def.rs

    poly_func: PolyFuncTypeRV,
-    #[serde(skip)]
-    pub(crate) validate: Box<dyn ValidateTypeArgs>,
+    pub(crate) validate: Option<Box<dyn ValidateTypeArgs>>,


If None here means "no custom validation required" then agreed, but in that case let's rename (or even inline) struct CustomValidator

a rename or inline would be a breaking change, which certainly doesn't have to be
avoided but in this case I think just enough docstrings should do the trick?

ss2165 · 2024-07-29T11:27:53Z

I intend to pull out the CLI extension dumping in to a follow up PR. In this PR will test that serialisations of standard extensions validate against schema. In follow up will validate against schema when generating from CLI + store cli-generated extensions in source tree + add a test that running generation matches stored extensions (an update-extensions script for updating them).

acl-cqc

So far looks good! I haven't seen the problem(s) yet, although I was thinking perhaps adding a struct for Hugr+extensions, with a validate method (maybe making a trait Validatable would be a good start), might come first

acl-cqc · 2024-07-29T11:20:38Z

hugr-core/src/extension/op_def.rs

    /// A custom binary which computes a polymorphic function type given values
    /// for its static type parameters.
    CustomFunc(Box<dyn CustomSignatureFunc>),
+    /// Declaration specified a custom binary but it was not provided.
+    MissingComputeFunc,


What about a missing custom validation func? In either case I think we just have to trust the cache, maybe with a warning, so I guess if the warning is the same then we don't have to distinguish, is that the plan?

I'm not sure - in the case of missing validation function do you trust the cached signature or use the type scheme to generate the signature and check against cache as you would without custom validation?

Good point. Given the validation function can only say one of two things - invalid, or use the type scheme - you can try the latter (which might reject if TypeArgs don't match the TypeParams, say) and see if that matches; that might get you an error, but even if the typescheme says ok, if there's a binary validation function that you haven't got, then that still has to be a warning

acl-cqc · 2024-07-29T11:21:45Z

hugr-core/src/extension/op_def.rs

+        /// If the type scheme is available explicitly, store it.
+        signature: Option<PolyFuncTypeRV>,
+        /// Whether an associated binary function is expected.
+        /// If `signature` is `None`, a true value here indicates a custom compute function.


Nice, and good docs, ok, that makes sense

acl-cqc · 2024-07-29T11:23:10Z

hugr-core/src/extension/op_def.rs

+    {
+        let SerSignatureFunc { signature, .. } = SerSignatureFunc::deserialize(deserializer)?;
+
+        // TODO for now, an indication of expected custom validation function is ignored.


Ah ok. Maybe just add a bool field in SignatureFunc::MissingComputeFunc (renaming to MissingFunc and using an enum rather than bool would be better)

I've decided to capture missing validation explicitly.

acl-cqc · 2024-07-29T11:24:15Z

hugr-core/src/extension/op_def.rs

+        let SerSignatureFunc { signature, .. } = SerSignatureFunc::deserialize(deserializer)?;
+
+        // TODO for now, an indication of expected custom validation function is ignored.
+        Ok(if let Some(sig) = signature {


nit - this is something like signature.map_or(MissingComputeFunc, super::SignatureFunc::From) where the MissingComputeFunc is the default for None

hugr-core/src/extension/op_def.rs

acl-cqc · 2024-07-29T11:26:24Z

hugr-core/src/extension/op_def.rs

-    fn static_params(&self) -> &[TypeParam] {
-        match self {
+    fn static_params(&self) -> Result<&[TypeParam], SignatureError> {
+        Ok(match self {
            SignatureFunc::PolyFuncType(ts) => ts.poly_func.params(),


aaiieeee is this one even correct? "static_params" are the ones to pass to binary compute_signature, so if there isn't such, I think this should be the empty list

yeah I found these functions aren't actually used (therefore aren't tested...) will add an issue.

Edit: #1375

acl-cqc · 2024-07-29T11:27:29Z

hugr-core/src/extension/type_def.rs

 pub enum TypeDefBound {
    /// Defined by an explicit bound.
-    Explicit(TypeBound),
+    Explicit { bound: TypeBound },


Presumably we will be able to stop doing these annoying renames-to-support serde when we have hugr-model :)

acl-cqc · 2024-07-29T11:33:33Z

Does a PR that just tackles "Extension+OpDef can state there is a binary compute_signature/validate function but we don't have it here" work on its own, and then enable separate/parallel follow-up PRs/threads of (a) serialization, cli, .... (b) remove opaqueop?

ss2165 · 2024-07-29T15:01:25Z

hugr-py/src/hugr/serialization/ops.py

-    hugr: Any
-
-
-class OpDef(ConfiguredBaseModel, populate_by_name=True):


moved to extension.py

Yes good call

…bility. BREAKING CHANGE: exernally tagged TypedDefBound serialisation will no longer work.

New "binary" field in serialisation may indicate that binary validation/computation functions are expected.

acl-cqc

Nice :). All major design considerations from earlier comments neatly addressed, thanks. Minor issues only, thanks!

(I think - if I'm barking up the wrong tree in places then feel free to ping me but I think you can probably take it from here)

acl-cqc · 2024-07-30T09:22:43Z

hugr-core/src/extension/op_def.rs

    poly_func: PolyFuncTypeRV,
-    #[serde(skip)]
+    //Optional custom function for validating type arguments before returning the signature.


This doesn't look very optional ?

Right, I'd started commenting in a different direction, but making this non-optional is a different way of doing what I was going to suggest, that works for me. Just correct the comment tho :)

acl-cqc · 2024-07-30T09:24:07Z

hugr-core/src/extension/op_def.rs

-    #[serde(skip)]
+    /// An explicit polymorphic function type.
+    PolyFuncType(PolyFuncTypeRV),
+    /// A polymorphic function type with a custom binary for validating type arguments.


maybe we could slightly emphasize that this is mostly the same as the previous case but PLUS a custom binary

acl-cqc · 2024-07-30T09:25:14Z

hugr-core/src/extension/op_def.rs

 }
+
+#[derive(PartialEq, Eq, Debug)]


Methinks we should not need NoValidate. One should use Signature::PolyFuncType instead.

yes, moved it just to the test that used it

acl-cqc · 2024-07-30T09:27:10Z

hugr-core/src/extension/op_def.rs

-            SignatureFunc::PolyFuncType(custom) => {
-                custom.validate.validate(args, def, exts)?;
+            SignatureFunc::CustomValidator(custom) => {
+                custom.validate.as_ref().validate(args, def, exts)?;


presumably this extra as_ref() is needed? Code looks very similar to what we had before.

wasn't needed, removed

acl-cqc · 2024-07-30T09:30:11Z

hugr-core/src/extension/op_def.rs

                // This is ruled out by `new()` but leave it here for later.
-                SignatureFunc::CustomFunc(_) => None,
+                SignatureFunc::CustomFunc(_) | SignatureFunc::MissingComputeFunc => None,


I suspect the comment on the previous line can go (I suspect "later" meant "when we have implemented serialization", essentially!), please clarify if I'm wrong

acl-cqc · 2024-07-30T09:35:31Z

hugr-core/src/hugr/serialize/test.rs

+    for (_, ext) in std_reg.into_iter() {
+        let val = serde_json::to_value(ext).unwrap();
+        NamedSchema::check_schemas(&val, get_schemas(true));
+        // check deserialises correctly, can't check equality because of custom binaries.


However I think you could try reserializing the deserialized Extension to check equality with the original?

deserialize(serialize(ext)) != ext // because custom binaries serialize(deserialize(serialize(ext))) == serialize(ext) // I think

yes nice thanks

acl-cqc · 2024-07-30T09:36:16Z

hugr-py/pyproject.toml

@@ -26,6 +26,8 @@ repository = "https://github.com/CQCL/hugr"
 [tool.poetry.dependencies]
 python = ">=3.10"
 pydantic = ">=2.7,<2.9"
+pydantic-extra-types = "^2.9.0"
+semver = "^3.0.2"


nit: does this belong in this PR?

Ah, you added Rust semver in an earlier PR but didn't update the python, and tests added here reveal this?

the earlier PR added versions to the rust struct, but there was not matching python model. This PR adds the python model, and with it the semver dependency for the python model.

acl-cqc · 2024-07-30T09:37:10Z

hugr-py/src/hugr/serialization/extension.py

+        return get_serialisation_version()
+
+    @classmethod
+    def _pydantic_rebuild(cls, config: pd.ConfigDict | None = None, **kwargs):


Might be good to comment why you need this for the uninitiated

acl-cqc · 2024-07-30T09:37:27Z

hugr-py/src/hugr/serialization/ops.py

-    hugr: Any
-
-
-class OpDef(ConfiguredBaseModel, populate_by_name=True):


Yes good call

hugr-core/src/extension/op_def.rs

## 🤖 New release * `hugr`: 0.10.0 -> 0.11.0 * `hugr-core`: 0.7.0 -> 0.8.0 * `hugr-passes`: 0.6.2 -> 0.7.0 * `hugr-cli`: 0.3.0 -> 0.4.0 <details><summary>Changelog</summary> ## `hugr` <blockquote> ## 0.11.0 (2024-08-12) ### Bug Fixes - [**breaking**] BasicBlockExits should not be `OpTag::DataflowParent` ([#1409](#1409)) ### Documentation - Clarify CustomConst::equal_consts ([#1396](#1396)) ### Features - [**breaking**] Serialised extensions ([#1371](#1371)) - Serialised standard extensions ([#1377](#1377)) - [**breaking**] Update remaining builder methods to "infer by default" ([#1386](#1386)) - Add Eq op to logic extension ([#1398](#1398)) - Improve error message on failed custom op validation ([#1416](#1416)) - [**breaking**] `Extension` requires a version ([#1367](#1367)) </blockquote> ## `hugr-core` <blockquote> ## 0.8.0 (2024-08-12) ### Bug Fixes - [**breaking**] BasicBlockExits should not be `OpTag::DataflowParent` ([#1409](#1409)) ### Documentation - Clarify CustomConst::equal_consts ([#1396](#1396)) ### Features - [**breaking**] `Extension` requires a version ([#1367](#1367)) - [**breaking**] Serialised extensions ([#1371](#1371)) - Serialised standard extensions ([#1377](#1377)) - [**breaking**] Update remaining builder methods to "infer by default" ([#1386](#1386)) - Add Eq op to logic extension ([#1398](#1398)) - Improve error message on failed custom op validation ([#1416](#1416)) </blockquote> ## `hugr-passes` <blockquote> ## 0.7.0 (2024-08-12) ### Features - [**breaking**] `Extension` requires a version ([#1367](#1367)) </blockquote> ## `hugr-cli` <blockquote> ## 0.4.0 (2024-08-12) ### Features - Serialised standard extensions ([#1377](#1377)) - Validate with extra extensions and packages ([#1389](#1389)) - [**breaking**] Move mermaid to own sub-command ([#1390](#1390)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). --------- Co-authored-by: Craig Roy <[email protected]>

🤖 I have created a release *beep* *boop* --- ## [0.6.0](hugr-py-v0.5.0...hugr-py-v0.6.0) (2024-08-12) ### ⚠ BREAKING CHANGES * **hugr-py:** Moved `hugr.get_serialization_version` to `hugr.serialization.serial_hugr.serialization_version` * **hugr-cli:** Cli validate command no longer has a mermaid option, use `mermaid` sub-command instead. * `TypeDefBound` uses struct-variants for serialization. `SignatureFunc` now has variants for missing binary functions, and serializes in to a new format that indicates expected binaries. ### Features * `Package` pydantic model for modules + extensions ([#1387](#1387)) ([68cfac5](68cfac5)), closes [#1358](#1358) * Define `Const` inline by default, and add a parameter to change the parent ([#1404](#1404)) ([3609736](3609736)) * **hugr-cli:** move mermaid to own sub-command ([#1390](#1390)) ([77795b9](77795b9)) * **hugr-py:** add type_bound method to `Type` ([#1410](#1410)) ([bd5ba47](bd5ba47)), closes [#1365](#1365) * **hugr-py:** Allow defining functions, consts, and aliases inside DFGs ([#1394](#1394)) ([d554072](d554072)) * **hugr-py:** Reexport commonly used classes from the package root ([#1393](#1393)) ([69925d0](69925d0)) * **py:** `Hugr.to_json` and `.load_json` helpers ([#1403](#1403)) ([e7f9f4c](e7f9f4c)) * **py:** Allow pre-declaring a `Function`'s output types ([#1417](#1417)) ([fa0f5a4](fa0f5a4)) * **py:** implement `iter` on `ToNode` ([#1399](#1399)) ([e88910b](e88910b)) * **py:** Parametric int type helper, and arbitrary width int constants ([#1406](#1406)) ([abd70c9](abd70c9)) * Serialised extensions ([#1371](#1371)) ([31be204](31be204)) ### Bug Fixes * **py:** `Hugr.__iter__` returning `NodeData | None` instead of `Node`s ([#1401](#1401)) ([c134584](c134584)) * **py:** Set output cont for Conditionals ([#1415](#1415)) ([67bb8a0](67bb8a0)) ### Documentation * **hugr-py:** expand toctree ([#1411](#1411)) ([aa81c9a](aa81c9a)) * **hugr-py:** remove multiversion + add justfile command ([#1381](#1381)) ([dd1dc48](dd1dc48)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

ss2165 changed the base branch from main to ss/ext-version July 26, 2024 14:46

ss2165 force-pushed the ss/ext-schema branch 2 times, most recently from 4a37118 to 4a3424a Compare July 29, 2024 11:03

acl-cqc reviewed Jul 29, 2024

View reviewed changes

ss2165 mentioned this pull request Jul 29, 2024

SignatureFunc::static_params is untested - may be bug #1375

Open

Base automatically changed from ss/ext-version to main July 29, 2024 12:51

ss2165 force-pushed the ss/ext-schema branch 2 times, most recently from 18872f4 to ee00843 Compare July 29, 2024 14:58

ss2165 commented Jul 29, 2024

View reviewed changes

ss2165 force-pushed the ss/ext-schema branch from ee00843 to 340e002 Compare July 29, 2024 15:04

ss2165 marked this pull request as ready for review July 29, 2024 15:11

ss2165 requested a review from a team as a code owner July 29, 2024 15:11

ss2165 requested review from aborgna-q and acl-cqc and removed request for aborgna-q July 29, 2024 15:11

ss2165 force-pushed the ss/ext-schema branch 3 times, most recently from 4540dfd to 6573761 Compare July 29, 2024 16:29

ss2165 added 8 commits July 30, 2024 10:12

refactor!: Use internal tagging for TypeDefBound for pydantic compati…

078da5d

…bility. BREAKING CHANGE: exernally tagged TypedDefBound serialisation will no longer work.

feat: pydantic models for extension definition

066dcde

feat: generate top level schemas including extensions

b9ae43a

feat!: Use custom serialization for SignatureFunc.

f175105

New "binary" field in serialisation may indicate that binary validation/computation functions are expected.

skip lower_funcs if empty

c5ab7d0

error rather than panic on missing function

0f3ec3e

add some explanation comments

6b923d8

CustomValidator docstrings

6b7c788

ss2165 added 4 commits July 30, 2024 10:12

feat: be explicit about missing validation

bf071dc

feat: extract serialisation in to module and add test

0e3b160

simplify ignore_missing_validation

e31839c

separate explicit and custom validate variants

a47252a

ss2165 force-pushed the ss/ext-schema branch from 6573761 to a47252a Compare July 30, 2024 09:12

fix CustomValidator docstrings

4cd9af6

acl-cqc approved these changes Jul 30, 2024

View reviewed changes

ss2165 added 5 commits July 30, 2024 10:55

minor review suggestions

21642ea

improve tests

8bcb85c

move NoValidate to test only

b243b17

remove unused method

7010d0e

convenience constructors for TypeDefBound

ac834de

ss2165 enabled auto-merge July 30, 2024 10:09

fix schema gen script

9990e78

ss2165 added this pull request to the merge queue Jul 30, 2024

Merged via the queue into main with commit 31be204 Jul 30, 2024
21 checks passed

ss2165 deleted the ss/ext-schema branch July 30, 2024 10:19

This was referenced Jul 30, 2024

chore(main): release hugr-py 0.6.0 #1379

Merged

chore: release #1376

Merged

hugrbot mentioned this pull request Aug 13, 2024

chore: release #1420

Closed

		hugr: Any


		class OpDef(ConfiguredBaseModel, populate_by_name=True):

feat!: Serialised extensions #1371

feat!: Serialised extensions #1371

Conversation

ss2165 commented Jul 26, 2024 • edited Loading

codecov bot commented Jul 26, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ss2165 commented Jul 29, 2024

acl-cqc 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

Choose a reason for hiding this comment

ss2165 Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc commented Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc Jul 30, 2024 • 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

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

ss2165 commented Jul 26, 2024 •

edited

Loading

codecov bot commented Jul 26, 2024 •

edited

Loading

ss2165 Jul 29, 2024 •

edited

Loading

acl-cqc commented Jul 29, 2024 •

edited

Loading

acl-cqc Jul 30, 2024 •

edited

Loading