Skip to content
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

Merged
merged 28 commits into from
Jun 19, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
80703e1
Remove Pattern
acl-cqc Jun 5, 2023
9ef30ad
Remove Pattern.rs, too skeletal to be useful
acl-cqc Jun 5, 2023
75b9f1f
Move rewrite to hugr/replace
acl-cqc Jun 5, 2023
5c5662f
Add RewriteOp enum, move Hugr code into RewriteOp::apply (a big match)
acl-cqc Jun 5, 2023
de70383
Make into a trait. So it has to be public...so what?
acl-cqc Jun 5, 2023
3a534e6
Parametrize by error type
acl-cqc Jun 5, 2023
55f83ba
fmt
acl-cqc Jun 5, 2023
76d13fe
Rename hugr/replace/{rewrite.rs -> replace.rs}
acl-cqc Jun 6, 2023
6e8b152
Rename src/hugr/{replace=>rewrite}(,.rs)
acl-cqc Jun 6, 2023
99b31ce
Rename Rewrite(Error) to Replace(Error)
acl-cqc Jun 6, 2023
070afdd
Rename RewriteOp to Rewrite
acl-cqc Jun 6, 2023
7348e11
Hugr::apply -> apply_rewrite
acl-cqc Jun 6, 2023
9349aa4
Merge remote-tracking branch 'origin/main' into refactor/replace_trait
acl-cqc Jun 6, 2023
ec8354e
Add may_fail_destructively check, default true, and Transactional wra…
acl-cqc Jun 6, 2023
147c937
is_err
acl-cqc Jun 6, 2023
9f1d382
unchanged_on_failure as trait associated constant
acl-cqc Jun 7, 2023
f0b5b82
Rephrase assert/debug_assert
acl-cqc Jun 7, 2023
45c5fc2
Merge remote-tracking branch 'origin/main' into refactor/replace_trait
acl-cqc Jun 7, 2023
8f8fcae
Merge remote-tracking branch 'origin/main' into refactor/replace_trait
acl-cqc Jun 9, 2023
4604c70
Move SimpleReplacement inside rewrite, move Hugr::apply_simple_replac…
acl-cqc Jun 9, 2023
a83a93e
unused variable
acl-cqc Jun 9, 2023
0291bba
Drive-by: simple_replace.rs: change ".ok();"s to unwrap
acl-cqc Jun 9, 2023
3572933
Merge remote-tracking branch 'origin/main' into refactor/replace_trait
acl-cqc Jun 9, 2023
04c8777
WIP
acl-cqc Jun 19, 2023
6070e20
Fix merge
acl-cqc Jun 19, 2023
4202f5e
Review comments
acl-cqc Jun 19, 2023
4553395
todo -> unimplemented, the plan is not necessarily to do these
acl-cqc Jun 19, 2023
0201233
fmt
acl-cqc Jun 19, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 5 additions & 24 deletions src/hugr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
mod hugrmut;

pub mod multiportgraph;
pub mod rewrite;
pub mod serialize;
pub mod validate;
pub mod view;
Expand All @@ -11,13 +12,14 @@ pub use self::hugrmut::HugrMut;
pub use self::validate::ValidationError;

use derive_more::From;
pub use rewrite::{Replace, ReplaceError, Rewrite};

use portgraph::dot::{hier_graph_dot_string_with, DotEdgeStyle};
use portgraph::{Hierarchy, PortGraph, UnmanagedDenseMap};
use thiserror::Error;

pub use self::view::HugrView;
use crate::ops::{ModuleOp, OpType};
use crate::rewrite::{Rewrite, RewriteError};
use crate::types::EdgeKind;

use html_escape::encode_text_to_string;
Expand Down Expand Up @@ -75,29 +77,8 @@ impl Hugr {
}

/// Applies a rewrite to the graph.
pub fn apply_rewrite(mut self, rewrite: Rewrite) -> Result<(), RewriteError> {
// Get the open graph for the rewrites, and a HUGR with the additional components.
let (rewrite, mut replacement, parents) = rewrite.into_parts();

// TODO: Use `parents` to update the hierarchy, and keep the internal hierarchy from `replacement`.
let _ = parents;

let node_inserted = |old, new| {
std::mem::swap(&mut self.op_types[new], &mut replacement.op_types[old]);
// TODO: metadata (Fn parameter ?)
};
rewrite.apply_with_callbacks(
&mut self.graph,
|_| {},
|_| {},
node_inserted,
|_, _| {},
|_, _| {},
)?;

// TODO: Check types

Ok(())
pub fn apply_rewrite<E>(&mut self, rw: impl Rewrite<E>) -> Result<(), E> {
rw.apply(self)
}

/// Return dot string showing underlying graph and hierarchy side by side.
Expand Down
14 changes: 14 additions & 0 deletions src/hugr/rewrite.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
//! Replace operations on the HUGR.

#[allow(clippy::module_inception)] // TODO: Rename?
acl-cqc marked this conversation as resolved.
Show resolved Hide resolved
pub mod replace;
use crate::Hugr;
pub use replace::{OpenHugr, Replace, ReplaceError};

/// An operation that can be applied to mutate a Hugr
pub trait Rewrite<E> {
acl-cqc marked this conversation as resolved.
Show resolved Hide resolved
/// Mutate the specified Hugr, or fail with an error.
/// Implementations are strongly encouraged not to mutate the Hugr
/// if they return an Err.
Copy link
Collaborator

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.

Copy link
Contributor Author

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....

  1. 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.
  2. 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.
  3. 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:
  4. 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)>

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...

  1. 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.
  2. 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

Copy link
Collaborator

@aborgna-q aborgna-q Jun 6, 2023

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.

  1. (3) (4) don't seem too practical without lots of extra overhead.
  1. May be an option for region-focused rewrites, but not necessarily for every rewrite.
  2. 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:

  1. 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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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()?

Copy link
Contributor Author

@acl-cqc acl-cqc Jun 7, 2023

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)

fn apply(self, h: &mut Hugr) -> Result<(), E>;
}
48 changes: 38 additions & 10 deletions src/rewrite/rewrite.rs → src/hugr/rewrite/replace.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
#![allow(missing_docs)]
//! Rewrite operations on Hugr graphs.
//! Replace operations on Hugr graphs.

use std::collections::HashMap;

use portgraph::substitute::OpenGraph;
use portgraph::{NodeIndex, PortIndex};
use thiserror::Error;

use super::Rewrite;
use crate::Hugr;

/// A subset of the nodes in a graph, and the ports that it is connected to.
Expand Down Expand Up @@ -76,7 +77,7 @@ pub type ParentsMap = HashMap<NodeIndex, NodeIndex>;
/// A rewrite operation that replaces a subgraph with another graph.
/// Includes the new weights for the nodes in the replacement graph.
#[derive(Debug, Clone)]
pub struct Rewrite {
pub struct Replace {
/// The subgraph to be replaced.
subgraph: BoundedSubgraph,
/// The replacement graph.
Expand All @@ -85,7 +86,7 @@ pub struct Rewrite {
parents: ParentsMap,
}

impl Rewrite {
impl Replace {
/// Creates a new rewrite operation.
pub fn new(
subgraph: BoundedSubgraph,
Expand Down Expand Up @@ -116,26 +117,53 @@ impl Rewrite {
///
/// This includes having a convex subgraph (TODO: include definition), and
/// having matching numbers of ports on the boundaries.
pub fn verify(&self) -> Result<(), RewriteError> {
pub fn verify(&self) -> Result<(), ReplaceError> {
self.verify_convexity()?;
self.verify_boundaries()?;
Ok(())
}

pub fn verify_convexity(&self) -> Result<(), RewriteError> {
pub fn verify_convexity(&self) -> Result<(), ReplaceError> {
todo!()
}

pub fn verify_boundaries(&self) -> Result<(), RewriteError> {
pub fn verify_boundaries(&self) -> Result<(), ReplaceError> {
todo!()
}
}

impl Rewrite<ReplaceError> for Replace {
/// Performs a Replace operation on the graph.
fn apply(self, h: &mut Hugr) -> Result<(), ReplaceError> {
// Get the open graph for the rewrites, and a HUGR with the additional components.
let (rewrite, mut replacement, parents) = self.into_parts();

// TODO: Use `parents` to update the hierarchy, and keep the internal hierarchy from `replacement`.
let _ = parents;

let node_inserted = |old, new| {
std::mem::swap(&mut h.op_types[new], &mut replacement.op_types[old]);
// TODO: metadata (Fn parameter ?)
};
rewrite.apply_with_callbacks(
&mut h.graph,
|_| {},
|_| {},
node_inserted,
|_, _| {},
|_, _| {},
)?;

// TODO: Check types
Ok(())
}
}

/// Error generated when a rewrite fails.
#[derive(Debug, Clone, Error, PartialEq, Eq)]
pub enum RewriteError {
/// The rewrite failed because the boundary defined by the
/// [`Rewrite`] could not be matched to the dangling ports of the
pub enum ReplaceError {
/// The replacement failed because the boundary defined by the
/// [`Replace`] could not be matched to the dangling ports of the
/// [`OpenHugr`].
#[error("The boundary defined by the rewrite could not be matched to the dangling ports of the OpenHugr")]
BoundarySize(#[source] portgraph::substitute::RewriteError),
Expand All @@ -150,7 +178,7 @@ pub enum RewriteError {
NotConvex(),
}

impl From<portgraph::substitute::RewriteError> for RewriteError {
impl From<portgraph::substitute::RewriteError> for ReplaceError {
fn from(e: portgraph::substitute::RewriteError) -> Self {
match e {
portgraph::substitute::RewriteError::BoundarySize => Self::BoundarySize(e),
Expand Down
4 changes: 1 addition & 3 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,8 @@ pub mod hugr;
pub mod macros;
pub mod ops;
pub mod resource;
pub mod rewrite;
pub mod types;
mod utils;

pub use crate::hugr::{Direction, Hugr, Node, Port, Wire};
pub use crate::hugr::{Direction, Hugr, Node, Port, Replace, ReplaceError, Wire};
pub use crate::resource::Resource;
pub use crate::rewrite::{Rewrite, RewriteError};
7 changes: 0 additions & 7 deletions src/rewrite.rs

This file was deleted.

10 changes: 0 additions & 10 deletions src/rewrite/pattern.rs

This file was deleted.