Skip to content

feat: Add PersistentHugr #2080

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

Open
wants to merge 44 commits into
base: main
Choose a base branch
from
Open

feat: Add PersistentHugr #2080

wants to merge 44 commits into from

Conversation

lmondada
Copy link
Contributor

@lmondada lmondada commented Apr 15, 2025

The base of this PR will be moved to main once #2070 has been merged in.

Status: The API and main structs have been defined. Most of the core logic has yet to be implemented.

Status: PR is ready. The trait implementations for HugrView, VerifyPatch and ApplyPatch will be in a separate PR once progress on those is unblocked.

Closes #2096

@lmondada lmondada changed the base branch from main to feat/rewrite-trait April 15, 2025 10:48
@lmondada lmondada force-pushed the feat/persistenthugr branch from 44c3fe9 to 5780cda Compare April 15, 2025 10:49
@lmondada lmondada requested a review from aborgna-q April 15, 2025 10:52
aborgna-q and others added 10 commits April 15, 2025 13:52
`insert_hugr`, `insert_from_view`, and `insert_subgraph` were written
before we made `Node` a type generic, and incorrectly assumed the return
type maps were always `hugr::Node`s.

The methods were either unusable or incorrect when using generic
`HugrView`s source/targets with non-base node types.

This PR fixes that, and additionally allows us us to have
`SiblingSubgraph::extract_subgraph` work for generic `HugrViews`.

BREAKING CHANGE: Added Node type parameters to extraction operations in
`HugrMut`.
`HugrMutInternals` is part of the semi-private traits defined in
`hugr-core`.
While most things get re-exported in `hugr`, we `*Internal` traits
require you to explicitly declare a dependency on the `-core` package
(as we don't want most users to have to interact with them).

For some reason there was a public re-export of the trait in a
re-exported module, so it ended up appearing in `hugr` anyways.

BREAKING CHANGE: Removed public re-export of `HugrMutInternals` from
`hugr`.
#2027 ended up being breaking due to adding a new variant to an error
enum missing the `non_exhaustive` marker.

This (breaking) PR makes sure all error enums have the flag.

BREAKING CHANGE: Marked all Error enums as `non_exhaustive`
* PartialValue now has a LoadedFunction variant, created by LoadFunction nodes (only, although other analyses are able to create PartialValues if they want)
* This requires adding a type parameter to PartialValue for the type of Node, which gets everywhere :-(.
* Use this to handle CallIndirects *with known targets* (it'll be a single known target or none at all) just like other Calls to the same function
* deprecate (and ignore) `value_from_function`
* Add a new trait `AsConcrete` for the result type of `PartialValue::try_into_concrete` and `PartialSum::try_into_sum`

Note almost no change to constant folding (only to drop impl of `value_from_function`)

BREAKING CHANGE: in dataflow framework, PartialValue now has additional variant; `try_into_concrete` requires the target type to implement AsConcrete.
Adds a generic node type to the `NodeHandle` type.

This is a required change for #2029.

drive-by: Implement the "Link the NodeHandles to the OpType" TODO
Closes #1595

BREAKING CHANGE: `values` field in `Extension` and `ExtensionValue`
struct/class removed in rust and python. Use 0-input ops that return
constant values.
@lmondada lmondada force-pushed the feat/persistenthugr branch from 5780cda to 6659c40 Compare April 17, 2025 13:14
@lmondada lmondada marked this pull request as ready for review April 17, 2025 13:17
@lmondada lmondada requested a review from a team as a code owner April 17, 2025 13:17
@lmondada lmondada force-pushed the feat/persistenthugr branch from 6659c40 to c480927 Compare April 17, 2025 13:23
@lmondada
Copy link
Contributor Author

bus factor considerations: we might want to fold RelRc into Hugr at some point (at least the parts that we care about), but I'd suggest first figuring out what works/if it works before putting efforts into that.

acl-cqc and others added 7 commits April 22, 2025 16:16
Currently We have several "passes": monomorphization, dead function
removal, constant folding. Each has its own code to allow setting a
validation level (before and after that pass).

This PR adds the ability chain (sequence) passes;, and to add validation
before+after any pass or sequence; and commons up validation code. The
top-level `constant_fold_pass` (etc.) functions are left as wrappers
that do a single pass with validation only in test.

I've left ConstFoldPass as always including DCE, but an alternative
could be to return a sequence of the two - ATM that means a tuple
`(ConstFoldPass, DeadCodeElimPass)`.

I also wondered about including a method `add_entry_point` in
ComposablePass (e.g. for ConstFoldPass, that means `with_inputs` but no
inputs, i.e. all Top). I feel this is not applicable to *all* passes,
but near enough. This could be done in a later PR but `add_entry_point`
would need a no-op default for that to be a non-breaking change. So if
we wouldn't be happy with the no-op default then I could just add it
here...

Finally...docs are extremely minimal ATM (this is hugr-passes), I am
hoping that most of this is reasonably obvious (it doesn't really do a
lot!), but please flag anything you think is particularly in need of a
doc comment!

BREAKING CHANGE: quite a lot of calls to current pass routines will
break, specific cases include (a) `with_validation_level` should be done
by wrapping a ValidatingPass around the receiver; (b) XXXPass::run()
requires `use ...ComposablePass` (however, such calls will cease to do
any validation).

closes #1832
…eady in the Hugr (#2094)

There are two issues:
* Errors. The previous NodeTemplates still always work, but the Call one
can fail if the Hugr doesn't contain the target function node. ATM there
is no channel for reporting that error so I've had to panic. Otherwise
it's an even-more-breaking change to add an error type to
`NodeTemplate::add()` and `NodeTemplate::add_hugr()`. Should we? (I note
`HugrMut::connect` panics if the node isn't there, but could make the
`NodeTemplate::add` builder method return a BuildError...and propagate
that everywhere of course)
* There's a big limitation in `linearize_array` that it'll break if the
*element* says it should be copied/discarded via a NodeTemplate::Call,
as `linearize_array` puts the elementwise copy/discard function into a
*nested Hugr* (`Value::Function`) that won't contain the function. This
could be fixed via lifting those to toplevel FuncDefns with
name-mangling, but I'd rather leave that for #2086 ....

BREAKING CHANGE: Add new variant NodeTemplate::Call; LinearizeError no
longer derives Eq.
- Allows `HugrMut` to be implemented for `HugrView`s with arbitrary node
types
- Removes `HugrMutInternals::hugr_mut(&mut self) -> &mut Hugr`, it can
be implemented for more complex types. This is required for #1926, but I
haven't touched the read-only side yet.
- Added a `Node` associated type to `Rewrite`. All existing rewrites
only implement `Rewrite<Node = Node>` for now, expanding their type is
left for a separate PR.

drive-by: Fix a couple bugs in rewrite implementations that assumed that
`SiblingMut` contained transitive children.

BREAKING CHANGE: `HugrMut` is now implemented generically for any
`HugrView::Node` type.
BREAKING CHANGE: `SiblingMut` has a new type parameter for the wrapped
hugr type.
Re-created from #2113, targeting the release branch instead.

BREAKING CHANGE: Downstream crates need to remove the model_unstable
feature flag when referencing hugr or hugr-core.

---------

Co-authored-by: Lukas Heidemann <[email protected]>
Closes #2077

BREAKING CHANGE: Removed `RootTagged` trait. Now `RootChecked` is a
non-hugrview wrapper only used to verify inputs where appropriate.
This PR splits the `Rewrite` trait into two (three) traits:
- a `VerifyPatch` trait that has the `fn verify` and `fn
invalidation_set` functions
- a `ApplyPatch` trait that has the `fn apply` function. This inherits
`VerifyPatch` and is the "rewriting" trait that should be used in most
scenarios.

In addition, there is a third trait `ApplyPatchHugrMut` that can be
implemented by any patches that can be applied to _any_ `HugrMut` (as
opposed to a specific type `H`). This is strictly stronger than
`ApplyPatch` and should be implemented instead of `ApplyPatch` where
possible (see the docs of the traits).

closes #588
closes #2052

BREAKING CHANGE: Replaced the `Rewrite` trait with `Patch`.
`Rewrite::ApplyResult` is now `Patch::Outcome`. `Rewrite::verify` was
split into a separate trait, and is now `PatchVerification::verify`.
BREAKING CHANGE: Renamed `hugr.rewrite` module to `hugr.patch`.
BREAKING CHANGE: Changed the type `OutlineCfg::ApplyResult` (now
`OutlineCfg::Outcome`) from `(Node, Node)` to `[Node; 2]`.

---------

Co-authored-by: Alan Lawrence <[email protected]>
Co-authored-by: Alan Lawrence <[email protected]>
@lmondada lmondada requested a review from cqc-alec April 29, 2025 15:08
lmondada and others added 4 commits April 29, 2025 17:16
Oupsie, during one of the merge conflict resolutions I must have
forgotten to remove the old `rewrite.rs` file. As you can see in the
`hugr-core/src/hugr.rs` file, this is no longer a module and thus the
file should be deleted. It has been renamed to `patch.rs` in #2070
This PR removes (runtime) extension sets from hugr-core and hugr-py (see
#1906).

BREAKING CHANGE: Functions that manipulate runtime extension sets have
been removed from the Rust and Python code. Extension set parameters
were removed from operations.
 
Closes #1906
Copy link
Collaborator

@cqc-alec cqc-alec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks great. Most of my comment are minor ones on the docs ... and some umming and aahing about the name.

@lmondada lmondada changed the base branch from release-rs-v0.16.0 to luca/repl-boundarymap May 4, 2025 15:09
@lmondada
Copy link
Contributor Author

lmondada commented May 4, 2025

Thanks @cqc-alec for the very useful comments, the code is much cleaner now. I think everything should be addressed.

deleted_nodes -> invalidation_set

I've realised in testing that I really do need the full invalidation_set as returned by SimpleReplacement (rather than the restricted set of deleted_nodes that I was using). This is because the output boundary map nu_out refers to the nodes past the output boundary; and thus cannot be modified by another patch.

I was trying to avoid using invalidation_set as it makes it impossible to apply two patches next to one another. I've resolved this by making SimpleReplacement more general in a separate PR, see #2151. EDIT: Done and merged into the release branch. I've temporarily rebased this PR on top of that one, and will undo this once #2151 is merged in.

Waiting before merging

I think I will hold off with merging this in until after the upcoming release: the changes in here are non-breaking and can be part of a future release. This will give me additional flexibility on breaking this API before it is released without requiring a further breaking release.

@lmondada lmondada force-pushed the feat/persistenthugr branch from 972c878 to 14f969a Compare May 6, 2025 07:39
@lmondada lmondada changed the base branch from luca/repl-boundarymap to luca/out-boundary-map May 6, 2025 07:39
@lmondada lmondada force-pushed the feat/persistenthugr branch from 61f1ea2 to 14f969a Compare May 6, 2025 08:01
Currently, SimpleReplacement stores its output boundary map nu_out by
referring to nodes outside the deleted subgraph in the host graph. This
forces the invalidation set of the replacements to include nodes past
the output, forbidding simultaneous adjacent replacements.

This PR fixes this by allowing the keys of `nu_out` (i.e. the ports on
the output boundary of the subgraph) to be either incoming ports (as
before), or outgoing ports (in which case this is equivalent to
specifying the map on all incoming ports linked to the given outgoing
ports). The latter is less general but covers most use cases and reduces
the size of the invalidation set.

Closes #2098

BREAKING CHANGE: Generalised arguments to [`SimpleReplacement::new`] and
[`SimpleReplacement::map_host_output`] to allow for outgoing ports.
Previous type signatures remain valid, however, type inference may fail.

---------

Co-authored-by: Mark Koch <[email protected]>
Base automatically changed from luca/out-boundary-map to release-rs-v0.16.0 May 6, 2025 11:00
mark-koch and others added 3 commits May 6, 2025 11:52
Feature branch for improved array lowering.

* The old `array` type is now called `value_array` and lives in a
separate extension
* The default `array` is now a linear type with additional `clone` and
`discard` operations
* To avoid code duplication, array operations and values are now defined
generically over a new `ArrayKind` trait that is instantiated with
`Array` (the linear one) and `VArray` (the copyable one) to generate the
`array` and `value_array` extensions
* An `array<n, T>` is now lowered to a fat pointer `{ptr, usize}` where
`ptr` is a heap allocated pointer of size at least `n * sizeof(T)` and
the `usize` is an offset pointing to the first element (i.e. the first
element is at `ptr + offset * sizeof(T)`). The rational behind the
additional offset is the `pop_left` operation which bumps the offset
instead of mutating the pointer. This way, we can still free the
original pointer when the array is discarded after a pop.

Tracked PRs:
* #2097 (closes #2066)
* #2100
* #2101
* #2110
* #2112 (closes #2067)
* #2119
* #2125 (closes #2124)

BREAKING CHANGE: `std.collections.array` is now a linear type, even if
the contained elements are copyable. Use the new
`std.collections.value_array` for an array with the previous copyable
semantics.

BREAKING CHANGE: `std.collections.array.get` now also returns the passed
array as an extra output

BREAKING CHANGE: `ArrayOpBuilder` was moved from
`hugr_core::std_extensions::collections::array::op_builder` to
`hugr_core::std_extensions::collections::array`.
for use in tket2/other downstream compilation tools
@lmondada lmondada requested a review from cqc-alec May 6, 2025 16:11
@lmondada lmondada force-pushed the feat/persistenthugr branch from c109959 to a7729f3 Compare May 6, 2025 16:44
@aborgna-q aborgna-q force-pushed the release-rs-v0.16.0 branch from 56c110c to 9cd24ff Compare May 7, 2025 10:54
Base automatically changed from release-rs-v0.16.0 to main May 7, 2025 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants