Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1325 +/- ##
==========================================
- Coverage 79.70% 79.69% -0.02%
==========================================
Files 160 158 -2
Lines 20595 20440 -155
Branches 19629 19474 -155
==========================================
- Hits 16415 16289 -126
+ Misses 3198 3171 -27
+ Partials 982 980 -2
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:
|
4c5fe2d to
83a50b4
Compare
| .insert_hugr(hugr.module_root(), func_def) | ||
| .inserted_entrypoint; | ||
| lowerer.replace_op(&op, NodeTemplate::Call(func_node, vec![])); | ||
| // TODO: Call is deprecated. We should use LinkedHugr instead. | ||
| #[expect(deprecated)] | ||
| lowerer.set_replace_op(&op, NodeTemplate::Call(func_node, vec![])); | ||
| } |
There was a problem hiding this comment.
Currently a TODO, should we update it before merging?
There was a problem hiding this comment.
It doesn't look that hard, given we have the func_def just above, but happy to leave, up to you. (If you want to fix, then I can give you a hand, you need a Hugr with a call node entrypoint, plus either func_def or a matching FuncDecl if you insert func_def above. Oh, and make sure the function in func_def is marked Visibility::Public!) Following #1333 we'll mark the new function private after linking so it can be removed (e.g. if no such op is ever lowered, or by LLVM after inlining) although I think we'll not RemoveDeadFuncs it ourselves (as RDF was earlier)...
There was a problem hiding this comment.
I was able to remove the other deprecated call, but doing the same thing here fails due to untranslated temp ops.
I guess it's because the hugrs inside the templates are not being translated in time when multiple lowerers are defined together...
There was a problem hiding this comment.
My quick read of the test lead me to expect something using LinkedHugr here:
tket2/tket-qsystem/src/extension/qsystem/lower.rs
Lines 138 to 141 in 8fa7eb6
acl-cqc
left a comment
There was a problem hiding this comment.
Ok so I am nervous of merging this without the nondeterminism bug solved; at the very least, we need to backup the branch so it isn't deleted by squash-merge, noting the "last deterministic commit".
I presume you want to merge before #1333 and then that can follow afterwards, and I guess that's ok as it's only a code-size regression.
But I'd also be happy to leave this as approved+mergable, but not merged, until such time as we desperately need it, and see whether that happens or we manage to fix the nondeterminism bug first....
| let mut b = ModuleBuilder::with_hugr(func_def); | ||
| let call = { | ||
| let mut f = b | ||
| .define_function_vis("", func_signature, Visibility::Private) |
There was a problem hiding this comment.
fwiw Private is default if you do just define_function, but fine for explicitness
| let mut call_hugr = b.finish_hugr().unwrap(); | ||
| call_hugr.set_entrypoint(call.node()); | ||
|
|
||
| NodeTemplate::LinkedHugr(Box::new(call_hugr), NameLinkingPolicy::default()) |
There was a problem hiding this comment.
NameLinkingPolicy::default will mean that if you insert the call-hugr more than once, you'll get an error that it's not sure which (existing or new) FuncDefn to use (it only wants to keep one and doesn't realize they are the same)...options are
- Use
default().on_multiple_defn(OnMultiDefn::UseTarget or UseSource) - Add a FuncDecl only, and then link in the FuncDefn once only either before or after ReplaceTypes
There was a problem hiding this comment.
Reverted this commit, but will take that into account for the follow up PR
| { | ||
| // TODO: Remove "llvm" feature gate once `inline_constant_functions` is moved to | ||
| // `hugr-passes`. See https://github.com/quantinuum/hugr/issues/2419 | ||
| // TODO: We still want to run this as long as deserialized hugrs are allowed to contain Value::Function |
There was a problem hiding this comment.
OIC, current implementation is in hugr::llvm, right. But no LLVM dependency there, so we could move it to hugr-passes. But we're just gonna delete it instead, right....
Ok, I guess there is nothing we can really do here, we have to leave it inside the #[cfg(feature = "llvm")]
| .insert_hugr(hugr.module_root(), func_def) | ||
| .inserted_entrypoint; | ||
| lowerer.replace_op(&op, NodeTemplate::Call(func_node, vec![])); | ||
| // TODO: Call is deprecated. We should use LinkedHugr instead. | ||
| #[expect(deprecated)] | ||
| lowerer.set_replace_op(&op, NodeTemplate::Call(func_node, vec![])); | ||
| } |
There was a problem hiding this comment.
My quick read of the test lead me to expect something using LinkedHugr here:
tket2/tket-qsystem/src/extension/qsystem/lower.rs
Lines 138 to 141 in 8fa7eb6
6960ec8 to
3b4efbe
Compare
3b4efbe to
c7a2221
Compare
acl-cqc
left a comment
There was a problem hiding this comment.
Ok, leaving both NodeTemplate::Calls in, and with the tests re-enabled, looks good - please go ahead ;-)
1b774f9 to
14dda69
Compare
I think this fixes the [nondeterminism observed](Quantinuum/tket2#1325 (comment)) in tket2#1325. (I updated tket2/Cargo.toml to use this branch, then ran `uv clean && uv pip install --reinstall tket selene-hugr-qis-compiler && uv sync` and that changed tests from failing to passing). Not sure I completely understand the mechanism - this appears to change only the order in which we add (all the outgoing edges from a node) for each node (not the order of outgoing edges for each), which AFAICS should have no effect. But....let's try....
Depends on the unreleased `hugr 0.25.0` Requires #1325
So following #1325 I was finally able to (/finally succeeded) in evaluating the effect of Quantinuum/hugr#2749 on guppy tests. (There was some question on that PR as to whether we needed to add new facility, see #2766; here is the data.) Three tests were affected (indeed the same ones as were broken and then fixed by Quantinuum/hugr#2779 but that is a separate issue). The sizes of the LLVM output as follows: | | Original (hugr 0.24.3) | Hugr 0.25+[fix](Quantinuum/hugr#2779) | Hugr 0.25+[fix](Quantinuum/hugr#2779) + this PR | |----|----|----|----| | [new test](Quantinuum/guppylang#1411) | 9568 | 11856 | 9888 | | basic_type | 10224 | 11616 | 10400 | | notebook2 | 15680 | 17184 | 15936 | | notebook5 (Hugr 1) | 10832 | 12304 | 11072 | | notebook5 (Hugr 2) | 10480 | 11840 | 10656 | Inspection of the hugrs in the middle row revealed the helper functions from Quantinuum/hugr#2749 were indeed present in the LLVM output as `define` (not `define private`). Hence, this PR, following which LLVM is able to remove the `define`s. (I have not looked into the remaining +2-3% increase. (The numbers include many other changes between hugr-0.24.3 and hugr-0.25.0.)
🤖 I have created a release *beep* *boop* --- ## [0.12.14](tket-py-v0.12.13...tket-py-v0.12.14) (2026-01-15) ### Features * make PytketHugrPass store a list of passes ([#1360](#1360)) ([498dd3a](498dd3a)) * Reduced glibc version requirement to 2.34 ([#1327](#1327)) ([498dd3a](498dd3a)) * Remove order edges in NormalizeGuppy pass ([#1326](#1326)) ([dbfffd5](dbfffd5)), closes [#1325](#1325) * Use constant folding by default on NormalizeGuppy ([#1323](#1323)) ([6f11024](6f11024)) ### Bug Fixes * Incorrect manylinux version in linux wheels ([#1327](#1327)) ([498dd3a](498dd3a)) * Wrongly reused qubit IDs in pytket encoding ([#1358](#1358)) ([498dd3a](498dd3a)) ### Documentation * update example notebook after Guppy optimization fixes ([#1357](#1357)) ([7487e3a](7487e3a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: Agustín Borgna <[email protected]>
## 🤖 New release * `tket`: 0.16.0 -> 0.17.0 (✓ API compatible changes) * `tket-qsystem`: 0.22.0 -> 0.23.0 (✓ API compatible changes) <details><summary><i><b>Changelog</b></i></summary><p> ## `tket` <blockquote> ## [0.17.0](tket-v0.16.0...tket-v0.17.0) - 2026-02-02 ### Bug Fixes - *(encoded-circ)* Track unsupported wires between input and output ([#1224](#1224)) - Multiple fixes to the pytket encoder ([#1226](#1226)) - Don't use opgroup in pytket barrier encoding ([#1251](#1251)) - guppy_to_circuit always returns num_operations = 0 ([#1200](#1200)) - *(pytket-decoder)* Avoid QAllocating and immediately freeing qubits ([#1256](#1256)) - Encoding of opaque subgraphs with no associated qubit/bit ([#1295](#1295)) - [**breaking**] Don't rely on command params for pytket barriers ([#1298](#1298)) - Track output qubits in CircuitInfo ([#1304](#1304)) - Wrongly reused qubit IDs in pytket encoding ([#1358](#1358)) ### New Features - Deprecate local find_tuple_unpack rewrite ([#1188](#1188)) - Add CopyableExpressionAST ([#1209](#1209)) - `NormalizeGuppy` pass to simplify generated structure ([#1220](#1220)) - [**breaking**] pytket EncodedCircuit struct for in-place pytket optimisation ([#1211](#1211)) - [**breaking**] Interval is independent of resource IDs and scope position ([#1205](#1205)) - Don't translate usizes to pytket ([#1241](#1241)) - BorrowSquashPass to elide redundant borrow/return ops ([#1159](#1159)) - [**breaking**] Bump hugr to 0.25.0 ([#1325](#1325)) - Remove order edges in NormalizeGuppy pass ([#1326](#1326)) - [**breaking**] Remove deprecated unpack tuple pass ([#1387](#1387)) ### Refactor - Remove contain_qubits, use TypeUnpacker ([#1283](#1283)) - [**breaking**] Replace Subcircuit with SiblingSubgraph ([#1288](#1288)) - *(metadata)* [**breaking**] Migrate all metadata keys onto the new metadata traits ([#1328](#1328)) </blockquote> ## `tket-qsystem` <blockquote> ## [0.23.0](tket-qsystem-v0.22.0...tket-qsystem-v0.23.0) - 2026-02-02 ### Bug Fixes - [**breaking**] Don't rely on command params for pytket barriers ([#1298](#1298)) - Wrongly reused qubit IDs in pytket encoding ([#1358](#1358)) ### New Features - `NormalizeGuppy` pass to simplify generated structure ([#1220](#1220)) - Allow running arbitrary serializable pytket passes on hugrs ([#1266](#1266)) - BorrowSquashPass to elide redundant borrow/return ops ([#1159](#1159)) - [**breaking**] Bump hugr to 0.25.0 ([#1325](#1325)) - Remove order edges in NormalizeGuppy pass ([#1326](#1326)) - hide new public funcs introduced by linearization ([#1333](#1333)) ### Testing - regenerate guppy_opt examples, and count gates ([#1249](#1249)) - run pytket on guppy_opt tests, measure (very limited) success ([#1250](#1250)) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/).
Reverted in Quantinuum#1325 due to a bug in hugr, now fixed in `0.25.2`. The change in `unpack_container` seems to cause issues since we are not lowering operations defined in the templates themselves recursively. --------- Co-authored-by: Alan Lawrence <[email protected]>
BREAKING CHANGE: Bumped hugr dependency to 0.25.0
BREAKING CHANGE: Bumped pyo3 dependency to 0.27.2
(Currently pinned to a commit in hugr main, will mark as ready once hugr is released)