test: serialisation round trip testing for `OpDef` #999

doug-q · 2024-05-03T13:17:17Z

We also add coverage for roundtripping CustomSerialized which was omitted from the previous PR.

mark-koch

The schema changes look good to me, I'm just a bit confused with the introduction of ConfiguredBaseModel in #982

mark-koch · 2024-05-07T10:08:42Z

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

+    @classmethod
+    def set_model_config(cls, config: ConfigDict):
+        cls.model_config = config.copy()
+        cls.model_config["extra"] = "allow"
+
+    class Config:
+        json_schema_extra = {
+            "required": ["c"],
+        }


I'm a bit confused with ConfiguredBaseModel, why do we need this?

I would have expected something like

class CustomConst(BaseMode, extra=Extra.allow): c: str

The ConfiguredBaseModel let's us generate multiple schemas with different settings. Unfortunately, those settings override the config you set up in the class. I suppose your suggestion would work (using BaseModel). It would not pick up any other (i.e. non-extra) config (e.g. strict = True, although I'm not sure that does anything) which I think would be very confusing if anyone ever used model_rebuild with more config.

But I am about to push changes here, which make this discussion moot.

acl-cqc

Generally looks good, a few thoughts on OpDefs

hugr/src/extension.rs

hugr/src/extension/op_def.rs

acl-cqc · 2024-05-10T09:03:37Z

hugr/src/extension/op_def.rs

+            } = &other.0;
+
+            let get_sig = |sf: &_| match sf {
+                // if SignatureFunc or CustomValidator are changed we should get


Is this a TODO or a description of what happens now? I don't think there is much we can do about changes to validators or custom binary signature-funcs, but we can't serialize those anyway....this looks good for serialization tests

We can't serialize them YET, but if we make them traits we can serialize them as we do CustomConst. This comment as intended to help people keep it correct when fields are added to CustomValidator.

acl-cqc · 2024-05-10T09:04:24Z

hugr/src/extension/op_def.rs

+                && misc == other_misc
+                && get_sig(signature_func) == get_sig(other_signature_func)
+                && get_lower_funcs(lower_funcs) == get_lower_funcs(other_lower_funcs)
+                && constant_folder.is_none()


If we serialize an OpDef with a constant folder, what do we expect to get back in the roundtrip? Might be worth a comment on these last two lines, or (if this code is used only for roundtrip testing) even an assert that they are none

Did you mean to do something here other than thumbs-up ?

Yes, I've added a comment

hugr/src/extension/op_def.rs

codecov · 2024-05-10T10:04:16Z

Codecov Report

Attention: Patch coverage is 97.71429% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 86.88%. Comparing base (d0cd023) to head (cab398c).

Files	Patch %	Lines
hugr-core/src/extension/op_def.rs	96.11%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #999      +/-   ##
==========================================
+ Coverage   86.78%   86.88%   +0.10%     
==========================================
  Files          91       91              
  Lines       18560    18697     +137     
  Branches    18167    18304     +137     
==========================================
+ Hits        16107    16245     +138     
+ Misses       1606     1603       -3     
- Partials      847      849       +2

Flag	Coverage Δ
rust	`86.96% <97.71%> (+0.10%)`	⬆️

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.

doug-q · 2024-05-14T15:32:44Z

This is ready for review, but it's based on #981 so must wait for that to be merged.

acl-cqc

Generally great ;-) (so yes that's even better than last time!). I think there's a bug in ConstInt64, and some minor suggestions of renames etc. for readability but looks good to me. Thanks @doug-q !

hugr-core/src/extension/op_def.rs

acl-cqc · 2024-05-29T11:19:08Z

hugr-core/src/extension/op_def.rs

+            fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy {
+                use crate::proptest::{any_serde_yaml_value, any_smolstr, any_string};
+                use proptest::collection::{hash_map, vec};
+                let signature_func: BoxedStrategy<SignatureFunc> = any::<SignatureFunc>();


super-nit: I'd inline signature_func and lower_func, they are barely shorter than their definitions

hugr-core/src/extension.rs

hugr-core/src/extension/op_def.rs

acl-cqc · 2024-05-29T11:47:26Z

hugr-core/src/ops/constant.rs

+                    3,  // No more than 3 branch levels deep
+                    32, // Target around 32 total elements
+                    3,  // Each collection is up to 3 elements long
+                    |element| {


element -> child maybe? It's the recursive call right?

You could, this is copy-pasted from the docs though.

acl-cqc · 2024-05-29T11:48:00Z

hugr-core/src/ops/constant.rs

+                    3,  // Each collection is up to 3 elements long
+                    |element| {
+                        (Type::any_non_row_var(), vec(element.clone(), 0..3)).prop_map(
+                            |(typ, contents)| {


and contents is the result of that recursive call (so elem_strat and elem, perhaps, or child_strat and child ?)

hugr-core/src/ops/constant.rs

acl-cqc · 2024-05-29T11:51:52Z

hugr-core/src/ops/constant/custom.rs

+                let typ = any::<Type>();
+                let extensions = any::<ExtensionSet>();
+                let value = (any_serde_yaml_value(), any_string()).prop_map(|(value, c)| {
+                    [("c".into(), c.into()), ("v".into(), value)]


Is it plausible to break this lambda |(value, c)| .... out into a function with a name and a type signature? It feels like it could be an any_something_with function (_with as it takes a couple of parameters)

Not quite sure what you mean. I don't think you can extract anything useful here. Happy to add a comment to explain what's going on.

acl-cqc · 2024-05-29T12:06:25Z

hugr-core/src/std_extensions/arithmetic/int_types.rs

+                let signed_strat = (..=LOG_WIDTH_MAX).prop_flat_map(|log_width| {
+                    use i64;
+                    let max_val = (2u64.pow(log_width as u32) / 2) as i64;
+                    let min_val = -max_val - 1;


Hang on. So let's take log_width == 5. Then max_val = 2u64.pow(5)/2 i.e. 16. min_val is therefore -17. You're then creating a value in range -17..16...

I'd not expect Self::new_s to be happy if you generated -17 or 16 though. But maybe you're not hitting this? I tried copying your max_val and min_val computation into a separate test and that lead me tothink my interpretation was correct...

(So, consider: rename max_val to max_valp1; then let min_val = -max_valp1; then use min_val..<max_valp1)

Yeah this doesn't make sense, will double check what's going on.

I've rewritten, and added a proptest that convinces me that it's now correct.

Co-authored-by: Alan Lawrence <alan.lawrence@quantinuum.com>

acl-cqc · 2024-05-30T08:16:18Z

hugr-core/src/extension/op_def.rs

@@ -474,12 +474,14 @@ impl Extension {
 }

 #[cfg(test)]
-mod test {
+pub mod test {


super-nit: pub(super) might have less possibility of misleading the reader

acl-cqc · 2024-05-30T08:17:38Z

hugr-core/src/extension/op_def.rs

+
+    impl SimpleOpDef {
+        pub fn new(op_def: OpDef) -> Self {
+            assert!(op_def.constant_folder.is_none());


Nice! I was going to comment before, that SimpleOpDef was actually OpDefEqualityWrapper and the "simple" only came from the impl Arbitrary for, but this enforces simplicity, like it :)

hugr-core/src/extension/op_def.rs

hugr-core/src/ops/constant.rs

acl-cqc · 2024-05-30T08:20:15Z

hugr-core/src/ops/constant.rs

+                    32, // Target around 32 total elements
+                    3,  // Each collection is up to 3 elements long
+                    |child_strat| {
+                        (Type::any_non_row_var(), vec(child_strat, 0..3)).prop_map(


That bit of renaming helps a lot here I think, thanks :)

acl-cqc · 2024-05-30T08:23:50Z

hugr-core/src/ops/constant/custom.rs

+                // The "c" and "v" come from the `typetag::serde` annotation on
+                // `trait CustomConst`.
+                // TODO This is not ideal, if we were to randomly
+                // generate a valid tag(e.g. "c" = "ConstInt") then things will


Suggested change

// generate a valid tag(e.g. "c" = "ConstInt") then things will

// generate a valid tag (e.g. "ConstInt") then things will

...in that the "c" = is not part of the tag. But the idea is that we are relying on the random any_string not matching any handler for a recognized type, right?

Perhaps change randomly on previous line to accidentally, and/or ...then the handler for that type will interpret the "v" and fail

acl-cqc · 2024-05-30T08:24:54Z

hugr-core/src/std_extensions/arithmetic/int_types.rs

+        fn any_signed_int_with_log_width() -> impl Strategy<Value = (u8, i64)> {
+            (..=LOG_WIDTH_MAX).prop_flat_map(|log_width| {
+                let width = 2u64.pow(log_width as u32);
+                let max_val = ((1u64 << (width - 1)) - 1u64) as i64;


Co-authored-by: Alan Lawrence <alan.lawrence@quantinuum.com>

## 🤖 New release * `hugr`: 0.5.0 -> 0.5.1 * `hugr-core`: 0.1.0 -> 0.2.0 * `hugr-passes`: 0.1.0 -> 0.2.0 * `hugr-cli`: 0.1.0 -> 0.1.1 <details><summary>Changelog</summary> ## `hugr` <blockquote> ## 0.5.1 (2024-06-07) ### Refactor - Move binary to hugr-cli ([#1134](#1134)) </blockquote> ## `hugr-core` <blockquote> ## 0.2.0 (2024-06-07) ### Bug Fixes - [**breaking**] Validate that control-flow outputs have exactly one successor ([#1144](#1144)) - Do not require matching extension_reqs when creating a replacement ([#1177](#1177)) ### Features - Add `ConstExternalSymbol` to prelude ([#1123](#1123)) - `HugrView::extract_hugr` to extract regions into owned hugrs. ([#1173](#1173)) ### Testing - Serialisation round trip testing for `OpDef` ([#999](#999)) </blockquote> ## `hugr-passes` <blockquote> ## 0.2.0 (2024-06-07) ### Features - Add `ValidationLevel` tooling and apply to `constant_fold_pass` ([#1035](#1035)) </blockquote> ## `hugr-cli` <blockquote> ## 0.1.1 (2024-06-07) ### Features - Reexport `clap::Parser` and `clap_verbosity_flag::Level` from hugr_cli ([#1146](#1146)) ### Refactor - Move binary to hugr-cli ([#1134](#1134)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/).

mark-koch reviewed May 7, 2024

View reviewed changes

doug-q force-pushed the feat/proptest2 branch from d8a4972 to 2ee4dff Compare May 7, 2024 16:18

doug-q force-pushed the feat/proptest-serialisation branch 3 times, most recently from c223a56 to c40b546 Compare May 8, 2024 10:00

doug-q force-pushed the feat/proptest2 branch from 73b700b to 070365b Compare May 8, 2024 14:15

doug-q changed the title ~~Feat/proptest2~~ fix: serialisation round trip testing for OpDef May 8, 2024

doug-q force-pushed the feat/proptest-serialisation branch 2 times, most recently from 74c6797 to 507d9c0 Compare May 8, 2024 14:37

doug-q mentioned this pull request May 8, 2024

fix!: OpDef serialisation #1013

Merged

acl-cqc reviewed May 10, 2024

View reviewed changes

doug-q force-pushed the feat/proptest-serialisation branch 3 times, most recently from 4d4fe77 to 5305fd2 Compare May 14, 2024 08:36

doug-q force-pushed the feat/proptest2 branch from 232ad37 to 578351e Compare May 14, 2024 15:24

doug-q force-pushed the feat/proptest-serialisation branch from 99c3fd2 to 950679c Compare May 14, 2024 15:27

doug-q force-pushed the feat/proptest2 branch from 578351e to 0ee7785 Compare May 14, 2024 15:30

doug-q marked this pull request as ready for review May 14, 2024 15:31

doug-q requested a review from acl-cqc May 14, 2024 15:32

This was referenced May 15, 2024

Tracking Issue: Property testing #1045

Open

floats do not perfectly round trip serialize. #1049

Closed

Tracking Issue: Serialisation #1050

Open

doug-q linked an issue May 15, 2024 that may be closed by this pull request

Use proptest over rstest cases to test Serialisation #983

Closed

doug-q force-pushed the feat/proptest-serialisation branch from 950679c to 86e5e11 Compare May 16, 2024 09:33

doug-q requested a review from a team as a code owner May 16, 2024 09:33

Base automatically changed from feat/proptest-serialisation to main May 16, 2024 10:20

doug-q added 2 commits May 20, 2024 10:14

opdef roundtrips

f523fc9

Add missing CustomSerialized generator

1011e7a

acl-cqc approved these changes May 29, 2024

View reviewed changes

doug-q and others added 4 commits May 29, 2024 13:11

Update hugr-core/src/extension/op_def.rs

3d9dbb0

Co-authored-by: Alan Lawrence <alan.lawrence@quantinuum.com>

wip

d9687e7

add a test for valid signed integers

be8e408

lints + format

96dc36b

doug-q force-pushed the feat/proptest2 branch from 8fd37d1 to 96dc36b Compare May 29, 2024 14:53

doug-q added 3 commits May 29, 2024 15:58

add comment

5756599

tweak

5e22501

tweak test

54b100e

acl-cqc approved these changes May 30, 2024

View reviewed changes

doug-q and others added 4 commits May 30, 2024 09:27

Update hugr-core/src/ops/constant.rs

0f1af4c

Co-authored-by: Alan Lawrence <alan.lawrence@quantinuum.com>

tweak

30a6d05

Merge branch 'feat/proptest2' of gh:CQCL-DEV/hugr into feat/proptest2

cdd82c1

Merge branch 'main' of gh:CQCL-DEV/hugr into feat/proptest2

cab398c

doug-q added this pull request to the merge queue May 30, 2024

doug-q removed this pull request from the merge queue due to a manual request May 30, 2024

doug-q changed the title ~~fix: serialisation round trip testing for OpDef~~ test: serialisation round trip testing for OpDef May 30, 2024

doug-q added this pull request to the merge queue May 30, 2024

Merged via the queue into main with commit d799f7f May 30, 2024
19 checks passed

doug-q deleted the feat/proptest2 branch May 30, 2024 09:07

This was referenced May 30, 2024

chore: release #1139

Closed

chore: release #1145

Closed

chore: release #1148

Closed

chore: release #1154

Closed

chore: release #1162

Closed

This was referenced Jun 6, 2024

chore: release #1170

Closed

chore: release #1178

Closed

chore: release #1180

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: serialisation round trip testing for `OpDef` #999

test: serialisation round trip testing for `OpDef` #999

doug-q commented May 3, 2024 •

edited

Loading

mark-koch left a comment

mark-koch May 7, 2024

doug-q May 7, 2024

doug-q May 7, 2024

acl-cqc left a comment

acl-cqc May 10, 2024

doug-q May 10, 2024

acl-cqc May 10, 2024

acl-cqc May 15, 2024

doug-q May 20, 2024

codecov bot commented May 10, 2024 •

edited

Loading

doug-q commented May 14, 2024

acl-cqc left a comment

acl-cqc May 29, 2024

acl-cqc May 29, 2024

doug-q May 29, 2024

acl-cqc May 29, 2024

acl-cqc May 29, 2024

doug-q May 29, 2024

acl-cqc May 29, 2024

doug-q May 29, 2024

doug-q May 29, 2024

acl-cqc May 30, 2024

acl-cqc May 30, 2024

acl-cqc May 30, 2024

acl-cqc May 30, 2024

acl-cqc May 30, 2024

	// generate a valid tag(e.g. "c" = "ConstInt") then things will
	// generate a valid tag (e.g. "ConstInt") then things will

test: serialisation round trip testing for OpDef #999

test: serialisation round trip testing for OpDef #999

Conversation

doug-q commented May 3, 2024 • edited Loading

mark-koch 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

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

codecov bot commented May 10, 2024 • edited Loading

Codecov Report

doug-q commented May 14, 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

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

test: serialisation round trip testing for `OpDef` #999

test: serialisation round trip testing for `OpDef` #999

doug-q commented May 3, 2024 •

edited

Loading

codecov bot commented May 10, 2024 •

edited

Loading