refactor: Direct import of model representation to Python #2683
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2683 +/- ##
==========================================
- Coverage 83.55% 83.35% -0.20%
==========================================
Files 266 267 +1
Lines 51761 52479 +718
Branches 47180 47190 +10
==========================================
+ Hits 43251 43746 +495
- Misses 6132 6355 +223
Partials 2378 2378
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:
|
| def string_to_package(string: str) -> hugr.model.Package: ... | ||
| def package_to_bytes(package: hugr.model.Package) -> bytes: ... | ||
| def bytes_to_package(binary: bytes) -> hugr.model.Package: ... | ||
| def bytes_to_package(binary: bytes) -> tuple[hugr.model.Package, bytes]: ... |
There was a problem hiding this comment.
shouldn't this be a breaking change?
There was a problem hiding this comment.
I think this doesn't count as a breaking change since the hugr._hugr module is underscored.
| continue | ||
|
|
||
| try: | ||
| decoded = json.loads(value) |
There was a problem hiding this comment.
I had no idea these definitions leaked outside the match block 🤯
I'd prefer if this logic was inside the first case branch anyway
There was a problem hiding this comment.
In two minds, but on balance I prefer this way, the advantage being much smaller match blocks and less indentation.
|
|
||
| def import_node_in_module(self, node: model.Node, link_prefix: int) -> Node | None: | ||
| """Import a model Node at the Hugr Module level.""" | ||
| self.link_prefix = link_prefix |
There was a problem hiding this comment.
After all the hugrs are imported I think this should be unset. Maybe it could be an explicit enter/exit paradigm like with enter_symbol instead of a field?
| case _: | ||
| error = f"Unexpected symbol in alias declaration: {symbol}" | ||
| raise ModelImportError(error) | ||
| return self.add_node( |
There was a problem hiding this comment.
Same comment as above: re: moving this logic into the relevant case branch
| raise ModelImportError(error) | ||
| return self.add_node( | ||
| node, | ||
| AliasDecl(alias=name, bound=TypeBound.Copyable), # TODO which bound? |
There was a problem hiding this comment.
Good question... Seems like something we should fix in the spec
There was a problem hiding this comment.
or add this field to model.DeclareAlias 🤔
There was a problem hiding this comment.
Will create an issue for this.
| """Import an entire CFG region from the model into the Hugr.""" | ||
| [entry_link] = region.sources | ||
| entry_block_idx = None | ||
| for i, child in enumerate(region.children): |
There was a problem hiding this comment.
Don't we have a spec invariant that entry,exit are the 0,1 children?
There was a problem hiding this comment.
Hmm, actually ... this is true in the hugr graph, and it does appear to be true in the model as well ... but I'm not sure that it's guaranteed in the latter. So prefer to leave like this just in case.
Closes #2287 .
Testing:
validate()perform a round-trip, checking that the hashes of the start and end Hugrs (computed using the_NodeHashmethod defined inconftest.py) agree.just export-integration-tests) and tried importing them (1) using the old method via json and (2) using the new method, and checked that the hashes of the two imported Hugrs agree (using the same_NodeHashmethod).There are a few TODO comments remaining in
load.pywhich I could not see a way to resolve using the existing model. I will investigate these further and raise issues if necessary.