test: proptests for round trip serialisation of `Type`s and `Op`s. #981

doug-q · 2024-04-30T16:04:22Z

Note that I make no attempt to generate "valid" types. In particular this means that type variables will be grossly inconsistent. It would be possible to improve this by passing a type environment through all the arbitrary instances in Parameters, although it's not clear to me just how far that would take us.

We can't use prop_recursive for mutually recursive types, so I have reimplemented it using Parameters.

codecov · 2024-04-30T16:32:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.55%. Comparing base (64cb43c) to head (871eb66).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #981      +/-   ##
==========================================
+ Coverage   86.20%   86.55%   +0.34%     
==========================================
  Files          82       83       +1     
  Lines       17236    17547     +311     
  Branches    17236    17547     +311     
==========================================
+ Hits        14859    15188     +329     
+ Misses       1556     1539      -17     
+ Partials      821      820       -1

Flag	Coverage Δ
rust	`86.55% <100.00%> (+0.34%)`	⬆️

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-01T08:29:19Z

Codecov Report

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

Somehow impl Arbitrary for TypeParam isn't covered.

acl-cqc

Looks good to me, many thanks @doug-q . It's a shame the code to come up with instances has to be so tricky but I think that's an inevitable result of using Rust rather than Haskell here ;-). My main thoughts are that

I don't really understand the details of the prop_recursive macros....
but maybe that's an easier way than having TypeDepth ??

hugr/Cargo.toml

hugr/src/hugr/ident.rs

acl-cqc · 2024-05-03T09:18:56Z

hugr/src/hugr/serialize/test.rs

@@ -64,6 +62,25 @@ include_schema!(
    "../../../../specification/schema/testing_hugr_schema_strict_v1.json"
 );

+macro_rules! impl_sertesting_from {


oh, I was kinda hoping we wouldn't need this anymore! Ah well

acl-cqc · 2024-05-03T09:36:49Z

hugr/src/hugr/serialize/test.rs

-        op: optype.into(),
-    });
+        #[test]
+        fn prop_roundtrip_optype(op in ((0..(std::u32::MAX / 2) as usize). prop_map(|x| portgraph::NodeIndex::new(x).into()), any::<Option<ExtensionSet>>(), any::<OpType>()).prop_map(|(parent, input_extensions, op)| NodeSer { parent, input_extensions, op })) {


This is a little hard to read...is it possible to outline this NodeSer-factory? I think the overall is fn prop_roundtrip_optype(op in <thing>) which is presumably a proptest-specific construct, but maybe that can be an Iterator<NodeSer> or something? (A shame to have to construct NodeSers really - ideally prop_roundtrip_optype should take an Optype, but I guess that doesn't fit the serialization format?)

acl-cqc · 2024-05-03T09:40:09Z

hugr/src/ops/constant.rs

+                let leaf_strat = prop_oneof![
+                    any::<ExtensionValue>().prop_map(|e| Self::Extension { e }),
+                    prop_oneof![
+                        // TODO we need an example of each legal root, in particular FuncDe{fn,cl}


I reckon in future we will have impl Arbitrary for Hugr - nothing says it has to be able to generate any Hugr, but we could cover a good cross-section - but very happy not to do that in this PR

acl-cqc · 2024-05-03T09:44:27Z

hugr/src/proptest.rs

+        ].sboxed()
+    };
+
+    static ref ANY_SERDE_YAML_VALUE_LEAF: SBoxedStrategy<serde_yaml::Value> = {


whoa, hang on. Surely we should not be constructing YAML this way. Arbitrary Rust object, serialize, validate against schema, deserialize, check equality ...?

This is used for the contents of CustomTypeArg and CustomSerialised, which really can be arbitrary.

Ah got it. I think that's worth a comment.

acl-cqc · 2024-05-03T09:47:54Z

hugr/src/types.rs

+        use crate::types::{CustomType, FunctionType, SumType, TypeBound, TypeEnum, TypeRow};
+        use ::proptest::prelude::*;
+        impl Arbitrary for super::TypeEnum {
+            type Parameters = TypeDepth;


Presumably you've done this because constructing arbitrary instances without enforcing a depth limit leaves to overly-massive/deep structures??

I wonder if an easier way might be just to probabilistically instruct the framework to prefer the leaf constructors over those with more nested components. Maybe that doesn't, or is hard to tune to, give sufficient coverage of the nested components!

Can you perhaps use the prop_recursive thing that appears to have a limit on depth and a target total size? Might that be a much easier way of getting structures of the shape you want?

acl-cqc · 2024-05-03T09:52:35Z

justfile

        "poetry run pytest"
    )

+proptest:
+    cargo test --all-features


R u sure you want to turn extension_inference on here too? ;-)

I guess this makes sense but I'm not 100% sure about the naming

acl-cqc · 2024-05-03T09:56:49Z

hugr/src/hugr/serialize/test.rs

+    proptest! {
+        #[test]
+        fn prop_roundtrip_type(t:  Type) {
+            check_testing_roundtrip(t)


Yay :). I was a bit confused when I looked at the wealth of methods with roundtrip in the name (it might help to rename ser_roundtrip_validate to ser_schema_roundtrip as it does not call validate), but this one looks to do what we want :-). You could remove the allow_unused on its declaration now!

It does not call validate, but it does validate against a schema. Leaving as is for now.

acl-cqc · 2024-05-03T10:01:44Z

We can't use prop_recursive for mutually recursive types, so I have reimplemented it using Parameters.

The logic for when to descend is not written down here(TODO), one wants to do so as little as possible while guaranteeing you do it at least once between recursing from one type into itself. Type being a glorfied newtype around TypeEnum is very useful here: descend in Type and, because nothing else has a TypeEnum, you don't have to descend anywhere else in the Type "heirarchy".

....but maybe I haven't understood this. You mean that prop_recursive only works for Rust types that directly reference themselves, rather than via other structures (e.g. SumType)? Ok, boo... [are we sure we can't get round this by defining a big factory method for a Type that constructs Sums, Customs, etc. all in the same factory?]

I was going to say that Hugr doesn't have recursive Types, but realize now that that's not useful...

The random type variables is fine as long as we don't validate the Types, and I agree we don't need to, so long as we check for equality against the original (which I think we do).

…tion (#1005) We fix `model_rebuild` in `tys.py` to update the `model_config` rather than overwrite it. This prevents our config, i.e. `json_scheme_extra.required` from being removed during a `model_rebuild`. We remove most `json_scheme_extra.required` from the schema, using these only for `RootModel`s. This allows us to remove `TaggedSumType`, as well as alleviating us from the need of introducing `TaggedOpaqueType`. The serialisation schema is updated, and is `proptest`ed in #981. Reviewers should verify that the `serde` annotations, the pydantic schema definition, and the generated schemas exactly match that branch. BREAKING CHANGE: * Serialization schema * `Const::const_type` and `Value::const_type` are renamed to `Const::get_type` and `Value::get_type`. These now match several other `get_type` functions. --------- Co-authored-by: Craig Roy <[email protected]>

doug-q · 2024-05-14T08:38:33Z

@acl-cqc I've addressed all your comments and tidied things up a bit.

I've used the proptest_derive macros wherever possible. What do you think? This makes it harder to modify the types without updating the arbitrary instance. For example: adding a variant to an enum without updating an Arbitrary impl would silently fail to ever generate that variant.

Also, I have enabled the "proptest" feature by default, because the proptesting is running in <1sec. CI build times are not a concern because caching. Perhaps we should remove the feature flag entirely? @ss2165

I will add OpDef in another PR.

ss2165 · 2024-05-14T10:14:05Z

The feature flag seems useful for dependencies to optionally use hugrs in their own proptests?

acl-cqc · 2024-05-15T13:00:23Z

hugr/Cargo.toml

@@ -22,7 +22,9 @@ bench = false
 path = "src/lib.rs"

 [features]
+default = ["proptest"]


Is it possible to turn the proptest feature on only when #[cfg(test)] is true, maybe?

Also pondering how well this agrees with what's in justfile

acl-cqc

No blockers, the proptest package doesn't give the easiest code to follow but I think I got there in the end. A few suggestions/places where comments might help. Also you should really global replace "heirarchy" with "hierarchy", the former is not a word in English ("heir" is about passing on your possessions when you die, "hierarchy" comes from Greek "hieros" = "ruler")...

RecursionDepth might be a good basis for adding legal typeparams, etc., in the future :). Are we sure we don't want to add a TypeBound in there (subsuming CustomTypeParameters)?
Those --feature flags are doing what we expect, right? cargo test on its own does not execute proptests?

acl-cqc · 2024-05-15T13:05:27Z

hugr/src/hugr/serialize/test.rs

-fn roundtrip_value(#[case] value: Value) {
-    check_testing_roundtrip(value);
-}
+    proptest! {


So we remove all the manual cases here. How confident are we that proptest covers the interesting ones? (And, what about the commented-out cases that fail. Do they no longer fail?)

this is a good point

acl-cqc · 2024-05-15T13:07:42Z

hugr/src/ops/custom.rs

@@ -165,6 +166,7 @@ impl From<ExtensionOp> for CustomOp {
 ///
 /// [Extension]: crate::Extension
 #[derive(Clone, Debug)]
+// #[cfg_attr(test, derive(proptest_derive::Arbitrary))]


Comment why not, or maybe you can enable this?

acl-cqc · 2024-05-15T13:11:48Z

hugr/src/proptest.rs

+/// [RecursionDepth].
+///
+/// Consider a path between two different nodes of the same type(e.g. two [Type]s, or two
+/// [FunctionType]s).  The path must be non-increasing in [RecursionDepth] because


non-increasing works as long as there is some probability of a decrease, and we'll probabilistically keep generating until we hit the option that decreases. That's not great - I'm mildly surprised to see that we don't insist that any directed Path between two nodes of the same kind (e.g. Type) decreases. Is that not possible?

We do insist on that. See the next paragraph.

acl-cqc · 2024-05-15T13:12:18Z

hugr/src/proptest.rs

+/// Consider the tree of values generated. Each node is labelled with a
+/// [RecursionDepth].
+///
+/// Consider a path between two different nodes of the same type(e.g. two [Type]s, or two


Suggested change

/// Consider a path between two different nodes of the same type(e.g. two [Type]s, or two

/// Consider a path between two different nodes of the same kind (e.g. two [Type]s, or two

acl-cqc · 2024-05-15T13:13:34Z

hugr/src/proptest.rs

+    }
+}
+
+impl<I: TryInto<usize>> From<I> for RecursionDepth


Are you using this? Lovely bit of parametrization, but are we really? :)

We are using it yes. Not strictly required but I think it's fine?

acl-cqc · 2024-05-15T13:48:45Z

hugr/src/types.rs

+    Sum(
+        #[cfg_attr(
+            all(test, feature = "proptest"),
+            proptest(strategy = "any_with::<SumType>(params)")


For my understanding - presumably this whole #[cfg_attr....] is necessary whenever you want to pass down the params ?

Well the cfg_attr is needed to apply the proptest(strategy = ... attribute conditionally. That attribute is needed because we are using non-default params, see paragraph 4 https://proptest-rs.github.io/proptest/proptest-derive/modifiers.html#params

acl-cqc · 2024-05-15T13:50:24Z

hugr/src/types/custom.rs

+                    vec(any_with::<TypeArg>(depth.descend()), 0..3).boxed()
+                };
+                (any_nonempty_string(), args, any::<ExtensionId>(), bound)
+                    .prop_map(|(id, args, extension, bound)| Self::new(id, args, extension, bound))


Suggested change

.prop_map(|(id, args, extension, bound)| Self::new(id, args, extension, bound))

.prop_map(Self::new)

or do you need the lambda to untuple?

It would be nice but we do need the lambda to untuple.

acl-cqc · 2024-05-15T13:53:29Z

hugr/src/types/custom.rs

+        use ::proptest::prelude::*;
+
+        #[derive(Default)]
+        pub struct CustomTypeArbitraryParameters(RecursionDepth, Option<TypeBound>);


So I wondered whether you could remove this Option but the answer is no because what you are selecting is (not a TypeBound but) a strategy for generating TypeBounds, i.e. potentially any::<TypeBound>.

acl-cqc · 2024-05-15T14:20:44Z

hugr/Cargo.toml

@@ -22,7 +22,9 @@ bench = false
 path = "src/lib.rs"

 [features]
+default = ["proptest"]


Also pondering how well this agrees with what's in justfile

acl-cqc · 2024-05-15T14:22:15Z

justfile

        "poetry run pytest"
    )

+# Run all proptests
+proptest:
+    cargo test --feature proptest '::proptest'


--feature proptest is on by default, no? So the '::proptest' here causes the tests to execute when cargo test (/ cargo test --feature proptest, which should be equivalent) does not?

acl-cqc · 2024-05-15T14:34:14Z

I think keeping proptests non-pub is sensible, the instances have significant "gotchas" like not being well-formed, we will need to consider carefully what we say when/if they become pub

Note that `impl Arbitrary for OpDef` is not yet implemented

doug-q · 2024-05-16T08:40:17Z

I am going to remove the feature flag, for the reasons noted in #981 (comment). Do lmk if you have a problem with this.

## 🤖 New release * `hugr`: 0.3.1 -> 0.4.0 (⚠️ API breaking changes) ### ⚠️ `hugr` breaking changes ``` --- failure inherent_method_const_removed: pub method is no longer const --- Description: A publicly-visible method or associated fn is no longer `const` and can no longer be used in a `const` context. ref: https://doc.rust-lang.org/reference/const_eval.html impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.31.0/src/lints/inherent_method_const_removed.ron Failed in: ConstF64::new in /tmp/.tmpwo5blB/hugr/hugr/src/std_extensions/arithmetic/float_types.rs:43 --- failure struct_missing: pub struct removed or renamed --- Description: A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.31.0/src/lints/struct_missing.ron Failed in: struct hugr::ops::constant::ExtensionValue, previously in file /tmp/.tmpq1W6bC/hugr/src/ops/constant.rs:184 ``` <details><summary>Changelog</summary> <blockquote> ## 0.4.0 (2024-05-20) ### Bug Fixes - Disallow non-finite values for `ConstF64` ([#1075](#1075)) - Serialization round-trips ([#948](#948)) - [**breaking**] Combine `ConstIntU` and `ConstIntS` ([#974](#974)) - Disable serialisation tests when miri is active ([#977](#977)) - [**breaking**] Serialisation schema ([#968](#968)) - Correct constant fold for `fne`. ([#995](#995)) - [**breaking**] Serialisation fixes ([#997](#997)) - [**breaking**] OpDef serialisation ([#1013](#1013)) - NaryLogicOp constant folding ([#1026](#1026)) ### Features - Add verification to constant folding ([#1030](#1030)) - Add `Const::get_custom_value` ([#1037](#1037)) - Add serialization schema for metadata ([#1038](#1038)) - 'Replace' rewrite returns node map ([#929](#929)) - `new` methods for leaf ops ([#940](#940)) - Add `string` type and `print` function to `prelude` ([#942](#942)) - `CustomOp::extension` utility function ([#951](#951)) - [**breaking**] Add `non_exhaustive` to various enums ([#952](#952)) - Encoder metadata in serialized hugr ([#955](#955)) - [**breaking**] Bring back Value ([#967](#967)) - Add LoadFunction node ([#947](#947)) - Add From impls for TypeArg ([#1002](#1002)) - Constant-folding of integer and logic operations ([#1009](#1009)) - [**breaking**] Update serialisation schema, implement `CustomConst` serialisation ([#1005](#1005)) - Merge basic blocks algorithm ([#956](#956)) - [**breaking**] Allow panic operation to have any input and output wires ([#1024](#1024)) ### Refactor - [**breaking**] Rename `crate::ops::constant::ExtensionValue` => `OpaqueValue` ([#1036](#1036)) - Outline hugr::serialize::test ([#976](#976)) - [**breaking**] Replace SmolStr identifiers with wrapper types. ([#959](#959)) - Separate extension validation from the rest ([#1011](#1011)) - Remove "trait TypeParametrised" ([#1019](#1019)) ### Testing - Reorg OutlineCfg/nest_cfgs tests so hugr doesn't depend on algorithm ([#1007](#1007)) - Ignore tests which depend on typetag when cfg(miri) ([#1051](#1051)) - Really ignore tests which depend on typetag when cfg(miri) ([#1058](#1058)) - Proptests for round trip serialisation of `Type`s and `Op`s. ([#981](#981)) - Add a test of instantiating an extension set ([#939](#939)) - Ignore serialisation tests when using miri ([#975](#975)) - [**breaking**] Test roundtrip serialisation against strict + lax schema ([#982](#982)) - Fix some bad assert_matches ([#1006](#1006)) - Expand test of instantiating extension sets ([#1003](#1003)) - Fix unconnected ports in extension test ([#1010](#1010)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Douglas Wilson <[email protected]>

doug-q force-pushed the feat/proptest-serialisation branch from ded0695 to e094512 Compare April 30, 2024 16:25

doug-q requested a review from acl-cqc May 1, 2024 08:20

doug-q force-pushed the feat/proptest-serialisation branch 4 times, most recently from 01e7a51 to c17885d Compare May 2, 2024 16:25

acl-cqc reviewed May 3, 2024

View reviewed changes

doug-q force-pushed the feat/proptest-serialisation branch 2 times, most recently from fe3450d to c223a56 Compare May 8, 2024 09:41

doug-q changed the base branch from main to feat/const-serialisation2 May 8, 2024 09:42

doug-q force-pushed the feat/const-serialisation2 branch from c8fdc48 to dbacfe0 Compare May 8, 2024 09:45

doug-q mentioned this pull request May 8, 2024

feat!: Update serialisation schema, implement CustomConst serialisation #1005

Merged

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

doug-q force-pushed the feat/const-serialisation2 branch from 3bc5749 to 8b7d7f4 Compare May 8, 2024 15:27

Base automatically changed from feat/const-serialisation2 to main May 9, 2024 16:24

doug-q force-pushed the feat/proptest-serialisation branch 2 times, most recently from 9d9e560 to 4d4fe77 Compare May 14, 2024 08:29

doug-q changed the title ~~test!: proptest serialisation of Types~~ test: proptests for round trip serialisation of Types and Ops. May 14, 2024

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

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

doug-q requested a review from acl-cqc May 14, 2024 08:36

This was referenced May 14, 2024

test: serialisation round trip testing for OpDef #999

Merged

Tracking Issue: Property testing #1045

Open

serde_yaml::Value does not perfectly round trip serialize. #1048

Closed

Tracking Issue: Serialisation #1050

Open

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

Add a just proptest recipe #994

Closed

acl-cqc reviewed May 15, 2024

View reviewed changes

acl-cqc approved these changes May 15, 2024

View reviewed changes

doug-q added 4 commits May 16, 2024 09:26

Add proptest for roundtrip testing of serialisation schema

8188b25

Note that `impl Arbitrary for OpDef` is not yet implemented

wip

37ad0af

tweak

17a49de

rebase

2ffeae9

doug-q added 3 commits May 16, 2024 10:11

address comments

2f8a0d3

remove feature "proptest"

ec79951

restore non-proptest test cases for serialisation

86e5e11

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

doug-q requested a review from mark-koch May 16, 2024 09:33

fix justfile

e155011

doug-q removed the request for review from mark-koch May 16, 2024 09:50

doug-q added 2 commits May 16, 2024 11:11

tidying

5f3d891

proptest deps -> dev-deps

871eb66

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

Merged via the queue into main with commit 0cc4030 May 16, 2024
18 checks passed

doug-q deleted the feat/proptest-serialisation branch May 16, 2024 10:20

This was referenced May 16, 2024

chore: release v0.4.0 #1031

Closed

chore: release v0.4.0 #1081

Merged

	/// Consider a path between two different nodes of the same type(e.g. two [Type]s, or two
	/// Consider a path between two different nodes of the same kind (e.g. two [Type]s, or two

	.prop_map(\|(id, args, extension, bound)\| Self::new(id, args, extension, bound))
	.prop_map(Self::new)

test: proptests for round trip serialisation of Types and Ops. #981

test: proptests for round trip serialisation of Types and Ops. #981

Conversation

doug-q commented Apr 30, 2024 • edited Loading

codecov bot commented Apr 30, 2024 • edited Loading

Codecov Report

doug-q commented May 1, 2024

Codecov Report

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

acl-cqc commented May 3, 2024

doug-q commented May 14, 2024 • edited Loading

ss2165 commented May 14, 2024

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

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

acl-cqc commented May 15, 2024

doug-q commented May 16, 2024

test: proptests for round trip serialisation of `Type`s and `Op`s. #981

test: proptests for round trip serialisation of `Type`s and `Op`s. #981

doug-q commented Apr 30, 2024 •

edited

Loading

codecov bot commented Apr 30, 2024 •

edited

Loading

doug-q commented May 14, 2024 •

edited

Loading