-
Notifications
You must be signed in to change notification settings - Fork 5
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
Refactor: move rewrite
inside hugr
, Rewrite
-> Replace
implementing new 'Rewrite' trait
#119
Conversation
src/hugr/rewrite.rs
Outdated
/// Implementations are strongly encouraged not to mutate the Hugr | ||
/// if they return an Err. |
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's quite restrictive. Normally we'd use ?
to resurface any internal errors during the rewriting. Those may happen at any point, so this would require doing (potentially costly) dry-checks before running.
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.
Yah this is tricky, and &mut Hugr -> Result<(), ...>
may not be the correct signature.
Firstly I note the "are strongly encouraged". So this is not actually a restriction at all. It was the best I could come up with given the open/unsealed trait, but it does make me think that all of the "primitive" rewrites we provide should meet this recommendation. Which....is indeed restrictive, yes.
Ok, so what are the options for what (can) happen if an error is raised. We don't have an abort
/commit
transaction-like semantics built in, so essentially this is where we are defining one....
- If an implementation raises an error, we could say the Hugr is in an undefined state. Without any guarantee, the Hugr is now basically useless, which is to say, you've lost your Hugr. IOW the signature should be
apply(Hugr) -> Result<Hugr, E>
.- Recovering from such situation basically means, the client restores the Hugr from a backup. Said client might be able to say, if that fails, I only need to recover this part of the Hugr, and only back up that part.
- Perhaps we insist that the Hugr, while it may have been mutated, still passes validate()? That could be extremely tricky to verify - you'd have to check failure at every possible point in the rewrite! - and still not terribly useful.
- Returning a Hugr where we can get back the original "by calling a no-args cleanup method" seems hard - such a cleanup method would not know what the rewrite did, so this will still be restrictive on the rewrite. Hence, I think we can dismiss that in favour of the strictly-more-flexible:
- We change "strongly encouraged" to "required". As noted, this is very demanding on implementations; they must either (a) make all checks before they begin, (b) record the changes made sufficiently to undo them - perhaps by performing in some order that makes this easy (e.g. if "removing all unconnected nodes" was sufficient to undo), (c) backup the Hugr before they start (perhaps they need only backup a small portion if they know they will only change that part of the Hugr).
- (b) and (c) might be easier here if, rather than leaving the Hugr unchanged upon failure, the impl instead provided a (perhaps-expensive) function to retrieve it if the client wants. I guess that'd be a signature like
apply(Hugr) -> Result<Hugr, (FnOnce<() -> Hugr>, E)>
- (b) and (c) might be easier here if, rather than leaving the Hugr unchanged upon failure, the impl instead provided a (perhaps-expensive) function to retrieve it if the client wants. I guess that'd be a signature like
Comparing 1. (client backups) and 4. (impl undoes), the former has the advantage that clients who know they can't continue if the rewrite fails, can perform the rewrite without incurring the overhead of backing up; whereas the latter has the advantage that surely the implementation knows better than the caller how to undo its own changes. However, there are more options...
- Perhaps what we are searching for is the ability to provide a localized mutable view onto part of the Hugr; if the rewrite fails, then that part is undefined (i.e. lost), but at least we can guarantee the rest is unchanged. Then, clients would know what part of the Hugr to backup if they wanted.
- Or, perhaps we need a separate trait method
validate(&Hugr) -> bool
. The contract could then be one of:- If the provided Hugr passes validate() before the rewrite is applied, and the rewrite validates on the Hugr, then applying the rewrite either succeeds or fails without changing anything (No guarantees if either Hugr or rewrite fails validation)
- If the provided Hugr passes validate() before the rewrite is applied, and the rewrite validates on the Hugr, then applying the rewrite succeeds; no guarantees if either Hugr or rewrite fails validation
- If the provided Hugr passes validate() before the rewrite is applied, and the rewrite validates on the Hugr, then applying the rewrite succeeds; if the Hugr validates but the rewrite does not validate, then applying the rewrite either succeeds or fails-without-changes; if the Hugr does not validate, no guarantees
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.
Transactional semantics is it's own beast. Anything beyond 1. will incur a hit to performance, that we may perhaps prefer the user to have control over.
- (3) (4) don't seem too practical without lots of extra overhead.
- May be an option for region-focused rewrites, but not necessarily for every rewrite.
- The pre-execution predicates are a thing in tket, and I think something we wanted to move away from.
I'd say the general rewrite interface shouldn't require this. I propose:
- Leave these assurances to each Replace operation. Add
Transactional<>
wrappers or perhaps specialized versions (e.g.struct TransactionalSimpleReplace(SimpleReplace)
) so the users that actually want to pay the cost can choose to.
@cqc-alec and @ss2165 may have opinions, but we could move this to a discussion too.
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.
Ok, I'm trying to go with 6. but do it better here. As per slack, this allows
- rewrites with trivial
may_fail_destructively()==false
if they can check ahead in apply() - rewrites with trivial
may_fail_destructively()==true
and no guarantees on apply() - A transactional wrapper to turn the second case into the first
- Other rewrites to do anything in between
- Clients to have guarantees if they want them (and pay for them when necessary), but not if they don't
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.
It's possible that after we've implemented all of our built-in primitive Rewrites we can reduce the allowable behaviours here but I'm keen to see how (painful) they turn out before we do that.
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.
Hmmm, perhaps I should state that apply
may panic
if-and-only-if the Hugr doesn't validate()?
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.
Ok, I think I've enacted as discussed. Associated constants still seem a bit WIP (see rust-lang/rust#92827), but the bulk of the work appears to have now been done on nightly (rust-lang/rust#70256)
rewrite
into hugr::replace
, add 'RewriteOp' trait for operations on Hugrrewrite
inside hugr
, Rewrite
-> Replace
implementing new 'Rewrite' trait
|
||
/// Checks whether the rewrite would succeed on the specified Hugr. | ||
/// If this call succeeds, [self.apply] should also succeed on the same `h` | ||
/// If this calls fails, [self.apply] would fail with the same error. |
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.
An alternative is to return Result<(), Option<E>>
where returning Err(None) means, no guarantee can be given about apply
. If a rewrite really has to get partway through in order to complete its own validity checks then said alternative would support that. But I think we should leave as Result<(), E>
until/unless we find a Rewrite where that's difficult.
(Compound Rewrites is one such case. For a sequence of rewrites, verify
might have to clone() the Hugr, and then step through the sequence, apply
ing each rewrite before verify
ing the next.)
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.
I'd say custom enums with {Recoverable, NonRecoverable}
variants rather than Option<E>
.
But yeah, let's keep it simple for now.
Ok, I've now merged Alec's SimpleReplace with this, thus applying the same refactoring to his code. I haven't managed to implement verify. I'm tempted to make
but I think that can be another PR (as it's somewhat contentious) - I'm hoping this isn't anymore |
src/hugr/rewrite/simple_replace.rs
Outdated
// Make self.parent the parent | ||
h.hierarchy | ||
.insert_after(new_node_index, self_input_node_index) | ||
.ok(); |
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.
@cqc-alec all this code was basically moved here, modulo renaming self
-> h
and r
-> self
. I'm not sure about this .ok()
though - that converts the Result to an Option, and then throws away. Should we '.unwrap'? Happy to make a driveby change in this PR if you agree
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.
Yes happy with the drive-by change.
src/hugr/rewrite/simple_replace.rs
Outdated
*new_node_successor_index, | ||
tgt_offset, | ||
) | ||
.ok(); |
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.
@cqc-alec and 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.
Main comment is replacing the generic parameter with an associated error type in Rewrite
.
Merging HEAD
may break some things.
|
||
/// Checks whether the rewrite would succeed on the specified Hugr. | ||
/// If this call succeeds, [self.apply] should also succeed on the same `h` | ||
/// If this calls fails, [self.apply] would fail with the same error. |
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.
I'd say custom enums with {Recoverable, NonRecoverable}
variants rather than Option<E>
.
But yeah, let's keep it simple for now.
## 🤖 New release * `quantinuum-hugr`: 0.1.0 <details><summary><i><b>Changelog</b></i></summary><p> <blockquote> ## 0.1.0 (2024-01-15) ### Bug Fixes - Subgraph boundaries with copies ([#440](#440)) - [**breaking**] Use internal tag for SumType enum serialisation ([#462](#462)) - Check kind before unwrap in insert_identity ([#475](#475)) - Allow for variables to get solved in inference ([#478](#478)) - In IdentityInsertion add noop to correct parent ([#477](#477)) - Failing release tests ([#485](#485)) - [**breaking**] Serialise `Value`, `PrimValue`, and `TypeArg` with internal tags ([#496](#496)) - Serialise custom constants with internal tag ([#502](#502)) - [**breaking**] Reduce max int width in arithmetic extension to 64 ([#504](#504)) - HugrView::get_function_type ([#507](#507)) - TODO in resolve_extension_ops: copy across input_extensions ([#510](#510)) - Use given input extensions in `define_function` ([#524](#524)) - Lessen requirements for hugrs in outline_cfg ([#528](#528)) - Make unification logic less strict ([#538](#538)) - Simple replace incorrectly copying metadata ([#545](#545)) - Account for self-referencial constraints ([#551](#551)) - Consider shunted metas when comparing equality ([#555](#555)) - Join labels in issue workflow ([#563](#563)) - Comment out broken priority code ([#562](#562)) - Handling of issues with no priority label ([#573](#573)) - Don't insert temporary wires when extracting a subgraph ([#582](#582)) - Improve convexity checking and fix test ([#585](#585)) - Ignore input->output links in SiblingSubgraph::try_new_dataflow_subgraph ([#589](#589)) - Enforce covariance of SiblingMut::RootHandle ([#594](#594)) - Erratic stack overflow in infer.rs (live_var) ([#638](#638)) - Work harder in variable instantiation ([#591](#591)) - Actually add the error type to prelude ([#672](#672)) - Serialise dynamically computed opaqueOp signatures ([#690](#690)) - FuncDefns don't require that their extensions match their children ([#688](#688)) - Binary compute_signature returning a PolyFuncType with binders ([#710](#710)) - Use correct number of args for int ops ([#723](#723)) - [**breaking**] Add serde tag to TypeParam enum ([#722](#722)) - Allow widening and narrowing to same width. ([#735](#735)) - Case node should not have an external signature ([#749](#749)) - [**breaking**] Normalize input/output value/static/other ports in `OpType` ([#783](#783)) - No dataflow_signature for block types ([#792](#792)) - Ignore unsupported test in miri ([#794](#794)) - Include schema rather than read file ([#807](#807)) ### Documentation - Add operation constraint to quantum extension ([#543](#543)) - Coverage check section in DEVELOPMENT.md ([#648](#648)) - Remove "quantum extension" from HUGR spec. ([#694](#694)) - Improve crate-level docs, including example code. ([#698](#698)) - Spec cleanups and clarifications ([#742](#742)) - Spec clarifications ([#738](#738)) - Spec updates ([#741](#741)) - [spec] Remove references to causal cone and Order edges from Input ([#762](#762)) - Mention experimental inference in readme ([#800](#800)) - Collection of spec updates for 0.1 ([#801](#801)) - Add schema v0 ([#805](#805)) - Update spec wrt. polymorphism ([#791](#791)) ### Features - Derive things for builder structs ([#229](#229)) - Return a slice of types from the signature ([#238](#238)) - Move `dot_string` to `HugrView` ([#271](#271)) - [**breaking**] Change `TypeParam::USize` to `TypeParam::BoundedNat` and use in int extensions ([#445](#445)) - TKET2 compatibility requirements ([#450](#450)) - Split methods between `HugrMut` and `HugrMutInternals` ([#441](#441)) - Add `HugrView::node_connections` to get all links between nodes ([#460](#460)) - Location information in extension inference error ([#464](#464)) - Add T, Tdg, X gates ([#466](#466)) - [**breaking**] Add `ApplyResult` associated type to `Rewrite` ([#472](#472)) - Implement rewrite `IdentityInsertion` ([#474](#474)) - Instantiate inferred extensions ([#461](#461)) - Default DFG builders to open extension sets ([#473](#473)) - Some helper methods ([#482](#482)) - Extension inference for conditional nodes ([#465](#465)) - Add ConvexChecker ([#487](#487)) - Add clippy lint for mut calls in a debug_assert ([#488](#488)) - Default more builder methods to open extension sets ([#492](#492)) - Port is serializable ([#489](#489)) - More general portgraph references ([#494](#494)) - Add extension deltas to CFG ops ([#503](#503)) - Implement petgraph traits on Hugr ([#498](#498)) - Make extension delta a parameter of CFG builders ([#514](#514)) - [**breaking**] SiblingSubgraph does not borrow Hugr ([#515](#515)) - Validate TypeArgs to ExtensionOp ([#509](#509)) - Derive Debug & Clone for `ExtensionRegistry`. ([#530](#530)) - Const nodes are built with some extension requirements ([#527](#527)) - Some python errors and bindings ([#533](#533)) - Insert_hugr/insert_view return node map ([#535](#535)) - Add `SiblingSubgraph::try_from_nodes_with_checker` ([#547](#547)) - PortIndex trait for undirected port parameters ([#553](#553)) - Insert/extract subgraphs from a HugrView ([#552](#552)) - Add `new_array` operation to prelude ([#544](#544)) - Add a `DataflowParentID` node handle ([#559](#559)) - Make TypeEnum and it's contents public ([#558](#558)) - Optional direction check when querying a port index ([#566](#566)) - Extension inference for CFGs ([#529](#529)) - Better subgraph verification errors ([#587](#587)) - Compute affected nodes for `SimpleReplacement` ([#600](#600)) - Move `SimpleReplace::invalidation_set` to the `Rewrite` trait ([#602](#602)) - [**breaking**] Resolve extension ops (mutating Hugr) in (infer_and_->)update_validate ([#603](#603)) - Add accessors for node index and const values ([#605](#605)) - [**breaking**] Expose the value of ConstUsize ([#621](#621)) - Nicer debug and display for core types ([#628](#628)) - [**breaking**] Static checking of Port direction ([#614](#614)) - Builder and HugrMut add_op_xxx default to open extensions ([#622](#622)) - [**breaking**] Add panicking integer division ops ([#625](#625)) - Add hashable `Angle` type to Quantum extension ([#608](#608)) - [**breaking**] Remove "rotations" extension. ([#645](#645)) - Port.as_directed to match on either direction ([#647](#647)) - Impl GraphRef for PetgraphWrapper ([#651](#651)) - Provide+implement Replace API ([#613](#613)) - Require the node's metadata to always be a Map ([#661](#661)) - [**breaking**] Polymorphic function types (inc OpDefs) using dyn trait ([#630](#630)) - Make prelude error type public ([#669](#669)) - Shorthand for retrieving custom constants from `Const`, `Value` ([#679](#679)) - [**breaking**] HugrView API improvements ([#680](#680)) - Make FuncDecl/FuncDefn polymorphic ([#692](#692)) - [**breaking**] Simplify `SignatureFunc` and add custom arg validation. ([#706](#706)) - [**breaking**] Drop the `pyo3` feature ([#717](#717)) - [**breaking**] `OpEnum` trait for common opdef functionality ([#721](#721)) - MakeRegisteredOp trait for easier registration ([#726](#726)) - Getter for `PolyFuncType::body` ([#727](#727)) - `Into<OpType>` for custom ops ([#731](#731)) - Always require a signature in `OpaqueOp` ([#732](#732)) - Values (and hence Consts) know their extensions ([#733](#733)) - [**breaking**] Use ConvexChecker trait ([#740](#740)) - Custom const for ERROR_TYPE ([#756](#756)) - Implement RemoveConst and RemoveConstIgnore ([#757](#757)) - Constant folding implemented for core and float extension ([#769](#769)) - Constant folding for arithmetic conversion operations ([#720](#720)) - DataflowParent trait for getting inner signatures ([#782](#782)) - Constant folding for logic extension ([#793](#793)) - Constant folding for list operations ([#795](#795)) - Add panic op to prelude ([#802](#802)) - Const::from_bool function ([#803](#803)) ### HugrMut - Validate nodes for set_metadata/get_metadata_mut, too ([#531](#531)) ### HugrView - Validate nodes, and remove Base ([#523](#523)) ### Miscellaneous Tasks - [**breaking**] Update portgraph 0.10 and pyo3 0.20 ([#612](#612)) - [**breaking**] Hike MSRV to 1.75 ([#761](#761)) ### Performance - Use lazy static definittion for prelude registry ([#481](#481)) ### Refactor - Move `rewrite` inside `hugr`, `Rewrite` -> `Replace` implementing new 'Rewrite' trait ([#119](#119)) - Use an excluded upper bound instead of max log width. ([#451](#451)) - Add extension info to `Conditional` and `Case` ([#463](#463)) - `ExtensionSolution` only consists of input extensions ([#480](#480)) - Remove builder from more DFG tests ([#490](#490)) - Separate hierarchy views ([#500](#500)) - [**breaking**] Use named struct for float constants ([#505](#505)) - Allow NodeType::new to take any Into<Option<ExtensionSet>> ([#511](#511)) - Move apply_rewrite from Hugr to HugrMut ([#519](#519)) - Use SiblingSubgraph in SimpleReplacement ([#517](#517)) - CFG takes a FunctionType ([#532](#532)) - Remove check_custom_impl by inlining into check_custom ([#604](#604)) - Insert_subgraph just return HashMap, make InsertionResult new_root compulsory ([#609](#609)) - [**breaking**] Rename predicate to TupleSum/UnitSum ([#557](#557)) - Simplify infer.rs/report_mismatch using early return ([#615](#615)) - Move the core types to their own module ([#627](#627)) - Change &NodeType->&OpType conversion into op() accessor ([#623](#623)) - Infer.rs 'fn results' ([#631](#631)) - Be safe ([#637](#637)) - NodeType constructors, adding new_auto ([#635](#635)) - Constraint::Plus stores an ExtensionSet, which is a BTreeSet ([#636](#636)) - [**breaking**] Remove `SignatureDescription` ([#644](#644)) - [**breaking**] Remove add_op_<posn> by generalizing add_node_<posn> with "impl Into" ([#642](#642)) - Rename accidentally-changed Extension::add_node_xxx back to add_op ([#659](#659)) - [**breaking**] Remove quantum extension ([#670](#670)) - Use type schemes in extension definitions wherever possible ([#678](#678)) - [**breaking**] Flatten `Prim(Type/Value)` in to parent enum ([#685](#685)) - [**breaking**] Rename `new_linear()` to `new_endo()`. ([#697](#697)) - Replace NodeType::signature() with io_extensions() ([#700](#700)) - Validate ExtensionRegistry when built, not as we build it ([#701](#701)) - [**breaking**] One way to add_op to extension ([#704](#704)) - Remove Signature struct ([#714](#714)) - Use `MakeOpDef` for int_ops ([#724](#724)) - [**breaking**] Use enum op traits for floats + conversions ([#755](#755)) - Avoid dynamic dispatch for non-folding operations ([#770](#770)) - Simplify removeconstignore verify ([#768](#768)) - [**breaking**] Unwrap BasicBlock enum ([#781](#781)) - Make clear const folding only for leaf ops ([#785](#785)) - [**breaking**] `s/RemoveConstIgnore/RemoveLoadConstant` ([#789](#789)) - Put extension inference behind a feature gate ([#786](#786)) ### SerSimpleType - Use Vec not TypeRow ([#381](#381)) ### SimpleReplace+OutlineCfg - Use HugrMut methods rather than .hierarchy/.graph ([#280](#280)) ### Testing - Update inference test to not use DFG builder ([#550](#550)) - Strengthen "failing_sccs_test", rename to "sccs" as it's not failing! ([#660](#660)) - [**breaking**] Improve coverage in signature and validate ([#643](#643)) - Use insta snapshots to add dot_string coverage ([#682](#682)) - Miri ignore file-opening test ([#684](#684)) - Unify the serialisation tests ([#730](#730)) - Add schema validation to roundtrips ([#806](#806)) ### `ConstValue - :F64` and `OpaqueOp::new` ([#206](#206)) ### Cleanup - Remove outdated comment ([#536](#536)) ### Cosmetic - Format + remove stray TODO ([#444](#444)) ### Doc - Crate name as README title + add reexport ([#199](#199)) ### S/EdgeKind - :Const/EdgeKind::Static/ ([#201](#201)) ### Simple_replace.rs - Use HugrMut::remove_node, includes clearing op_types ([#242](#242)) ### Spec - Remove "Draft 3" from title of spec document. ([#590](#590)) - Rephrase confusing paragraph about TailLoop inputs/outputs ([#567](#567)) ### Src/ops/validate.rs - Common-up some type row calculations ([#254](#254)) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). --------- Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Agustín Borgna <121866228+aborgna-q@users.noreply.github.com>
hugr::replace
packageAn alternative scheme is to use a fixed
enum
of the allowable operations: https://github.com/CQCL-DEV/hugr/compare/refactor/replace - but I think the trait version is better because:trait
altogether and have a bunch of "unrelated" methods...