-
Notifications
You must be signed in to change notification settings - Fork 13
test: Better python roundtrip tests, and lots of fixes #2436
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2436 +/- ##
==========================================
- Coverage 82.56% 81.99% -0.57%
==========================================
Files 247 247
Lines 45892 45938 +46
Branches 41528 41528
==========================================
- Hits 37889 37667 -222
- Misses 5981 6249 +268
Partials 2022 2022
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:
|
33fc29e to
f56283f
Compare
f56283f to
49c68c0
Compare
mark-koch
left a comment
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.
hugr-py/tests/test_val.py
Outdated
| assert repr(sum_val) == repr_str | ||
|
|
||
|
|
||
| @pytest.mark.skip("FIXME: static array value does not roundtrip-serialize correctly") |
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.
Is there an issue for this?
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.
Fixed the issue and removed the skip.
|
|
||
|
|
||
| @dataclass(frozen=True, order=True) | ||
| class _NodeHash: |
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 the hash also include the links? E.g. a list of successor hashes here?
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.
That would break with non-dag regions.
Since this is a hash of the whole hugr, including CFGs, I had avoid node orderings
hugr-py/src/hugr/ops.py
Outdated
| case InPort(_, -1) | OutPort(_, -1): | ||
| return tys.OrderKind() |
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.
Do we allow order edges on consts?
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.
Ah, hugr-rs doesn't allow it. Rolling back this change.
Apparently Input and Output restrict in/out order edges to only one direction, so I'll fix that too.
The eager encoding of `StaticArrayVal`'s payload caused the nested values to be encoded as _escaped_ strings, causing errors on the rust decoder. This PR rollbacks the changes to `StaticArrayVal` done in #2436 and adds a better workaround in the roundtrip test instead.
The python validation tests did a roundtrip serialization check by encoding the HUGR, loading it, encoding it again, and checking for differences in the serialization.
This ignored any information loss in the first encoding, and resulted in a lot of hidden bugs.
This PR changes the test in
conftest.pyto instead compare the original HUGR and the loaded one directly, using a "node hash" that computes the main properties of each node and its children in an index-independent way. (we do not traverse graph edges to avoid having a graph isomorphism problem).The node hash is defined as follows, and should be easily extensible if needed. We compare hugrs by checking the hashes of their root modules.
This revealed a bunch of bugs with the json serialization, hugr builders, node iterators, ...:
nullfor order edges, but python emitted#portsinstead (and this caused problems when combined with the out port inconsistencies below).Some(TRUE)it still shows asSomeinstead ofSum(tag=1, tys=[[], [Bool]], val=[TRUE]).Hugr.num_outgoingandnum_incomingwere counting ports instead of edges...FuncDefn,Case, and the likes.The roundtrip checks also have options for checking all the format combinations (one variable for the encoding format and another for converting it with
hugr convertbefore loading if necessary).This only does
json-jsonfor the moment, ashugr-modeldetects multiple errors.