feat!: Infer extension deltas for Case, Cfg, Conditional, DataflowBlock, Dfg, TailLoop #1195

acl-cqc · 2024-06-13T15:02:55Z

Add an ExtensionId TO_BE_INFERRED
On Case, Cfg, Conditional, DataflowBlock, Dfg, TailLoop, Hugr::infer_extensions replaces TO_BE_INFERRED with the smallest set of ExtensionIds that's correct wrt. its child nodes (i.e., the union of the child node deltas). If there are other ExtensionIds alongside TO_BE_INFERRED these will be kept (allowing a "lower bound" to be specified for individual nodes)
Hugr::infer_extensions also takes a remove: bool which, if true, modifies deltas of the same OpTypes that do not have TO_BE_INFERRED, by removing ExtensionIds. (Thus, non-inferred deltas act as upper bounds).

Relates to #640 but does not address the builder where this should be the default behaviour.

BREAKING CHANGE: ExtensionError moved from hugr::validate to hugr; Hugr::infer_extensions takes bool parameter

codecov · 2024-06-13T15:09:04Z

Codecov Report

Attention: Patch coverage is 96.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 86.66%. Comparing base (ea5213d) to head (4c5bf9c).
Report is 15 commits behind head on main.

Files	Patch %	Lines
hugr-core/src/hugr.rs	96.58%	1 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1195      +/-   ##
==========================================
+ Coverage   86.57%   86.66%   +0.08%     
==========================================
  Files          94       94              
  Lines       17659    17806     +147     
  Branches    16797    16944     +147     
==========================================
+ Hits        15289    15432     +143     
+ Misses       1600     1598       -2     
- Partials      770      776       +6

Flag	Coverage Δ
rust	`86.53% <96.66%> (+0.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

acl-cqc · 2024-06-13T16:15:20Z

hugr-core/src/hugr.rs

            self.validate_extensions()?;
        }
        Ok(())
    }

    /// Leaving this here as in the future we plan for it to infer deltas
    /// of container nodes e.g. [OpType::DFG]. For the moment it does nothing.
-    pub fn infer_extensions(&mut self) -> Result<(), ExtensionError> {
+    pub fn infer_extensions(&mut self, remove: bool) -> Result<(), ExtensionError> {
+        fn delta_mut(optype: &mut OpType) -> Option<&mut ExtensionSet> {


I guess this (+tests) might be better in a separate file, but this is 200 lines, a lot shorter than the old infer.rs

ss2165

The test simplifications and the brevity of the actual infer routine are very satisfying.

Thoughts on builder interface:

Builders that infer extensions should be default, therefore they should have the short names (probably just the set of names we currently use with explicit deltas).
Explicit delta versions, if necessary, can have some standard naming convention (e.g. dfg_builder_delta)
Could a potential simplification be just to require FunctionType inputs wherever possible (rather than in_types + out_types) and then make the default extension set in FunctionType TO_BE_INFERRED. Then with_extension_delta on FunctionType would overwrite the default set? Are there uses of FunctionType where having the default extension set be empty is important?

ss2165 · 2024-06-21T12:48:38Z

hugr-core/src/hugr/validate.rs

@@ -815,15 +816,5 @@ pub enum InterGraphEdgeError {
    },
 }

-#[derive(Debug, Clone, PartialEq, Error)]


breaking so needs mentioning in PR commit name + description

ss2165 · 2024-06-21T12:49:12Z

hugr-core/src/extension.rs

@@ -406,6 +406,8 @@ pub enum ExtensionBuildError {
 pub struct ExtensionSet(BTreeSet<ExtensionId>);

 impl ExtensionSet {
+    pub const TO_BE_INFERRED: ExtensionId = ExtensionId::new_unchecked("TO_BE_INFERRED");


maybe we want an even harder to hit name

Made an "illegal" identifier i.e. you can only create it via new_unchecked and not via new

ss2165 · 2024-06-21T12:52:16Z

hugr-core/src/hugr/validate.rs

@@ -743,6 +747,9 @@ pub enum ValidationError {
    /// There are errors in the extension deltas.
    #[error(transparent)]
    ExtensionError(#[from] ExtensionError),
+    /// A node claims to still be awaiting extension inference. Perhaps it is not acted upon by inference...


Suggested change

/// A node claims to still be awaiting extension inference. Perhaps it is not acted upon by inference...

/// A node claims to still be awaiting extension inference. Perhaps it is not acted upon by inference.

Fair, updated message too.

However, just to note - any Hugr-node that contains TO_BE_INFERRED in its delta is going to either be an error (if its parent doesn't) or result in a TLD that is basically unusable (because no runtime/environment/run_on_quantum_hw node/etc. will support such extensions). So we could skip both the error and the check. Just a thought....

ss2165 · 2024-06-21T12:54:02Z

hugr-core/src/hugr.rs

@@ -92,15 +91,63 @@ impl Hugr {
        self.validate_no_extensions(extension_registry)?;
        #[cfg(feature = "extension_inference")]
        {
-            self.infer_extensions()?;
+            self.infer_extensions(false)?;
            self.validate_extensions()?;
        }
        Ok(())
    }

    /// Leaving this here as in the future we plan for it to infer deltas


update docstring (incl what "remove" means)

Ooops, thanks

ss2165 · 2024-06-21T12:57:24Z

hugr-core/src/hugr.rs

+                .map(|ch| Ok((ch, infer(h, ch, remove)?)))
+                .collect::<Result<Vec<_>, _>>()?;
+
+            let Some(es) = delta_mut(h.op_types.get_mut(node.pg_index())) else {


should we just add a get_optype_mut(Node) to HugrMut?

Maybe, but

This raises tricky questions of what to do for a Node that is not in the Hugr. What'll actually happen here is we'll fall back to DenseUnmanagedMap's behaviour of expanding to contain an OpType for that Node (index), which I suspect is not what we'd want on a (quasi-?)public method...so at the least, it's a bit more involved that it sounds

If we do that, we should probably remove replace_op as having full &mut access is clearly superior! (replace_op(idx, op) is get_mut(idx) = op). I guess that could follow, but obviously it's a breaking change, and I think better to do that all in another PR.

I think this is fine for inside crate::hugr. (h is just self but we're in an inner-fn)

ss2165 · 2024-06-21T13:27:44Z

hugr-core/src/hugr.rs

+        assert_eq!(inf_res, Err(expected_err));
+    }
+
+    fn build_ext_cfg(parent: ExtensionSet) -> (Hugr, Node) {


Suggested change

fn build_ext_cfg(parent: ExtensionSet) -> (Hugr, Node) {

fn build_ext_dfg(parent: ExtensionSet) -> (Hugr, Node) {

?

Erm, yes, oops, no idea how that happened

ss2165 · 2024-06-21T13:29:48Z

hugr-core/src/hugr.rs

+        mid
+    }
+
+    #[rstest]


i think these cases would benefit from comments next to each saying briefly why the result is expected

ss2165 · 2024-06-21T13:32:06Z

hugr-core/src/hugr.rs

+                ..root_ty
+            };
+            assert!(h.root_type() == &expected_gp.into())
+            // rest should be unchanged...


worth asserting?

acl-cqc · 2024-06-24T10:16:58Z

Thoughts on builder interface:

Builders that infer extensions should be default, therefore they should have the short names (probably just the set of names we currently use with explicit deltas).

Explicit delta versions, if necessary, can have some standard naming convention (e.g. dfg_builder_delta)

Could a potential simplification be just to require FunctionType inputs wherever possible (rather than in_types + out_types) and then make the default extension set in FunctionType TO_BE_INFERRED. Then with_extension_delta on FunctionType would overwrite the default set? Are there uses of FunctionType where having the default extension set be empty is important?

Ok, moved to #1219

ss2165

thanks!

ss2165 · 2024-06-24T13:48:01Z

hugr-core/src/hugr.rs

+    #[case([XA], [TO_BE_INFERRED], true, [XA])]
+    // Success: delta inferred for parent is subset of grandparent
+    #[case([XA, XB], [TO_BE_INFERRED], true, [XA])]
+    // Base case failure: infers [XA] for parent but grandparent has disjoint et


Suggested change

// Base case failure: infers [XA] for parent but grandparent has disjoint et

// Base case failure: infers [XA] for parent but grandparent has disjoint set

…ck, Dfg, TailLoop (#1195) closes #640 * Add an ExtensionId TO_BE_INFERRED * On Case, Cfg, Conditional, DataflowBlock, Dfg, TailLoop, `Hugr::infer_extensions` replaces TO_BE_INFERRED with the smallest set of ExtensionIds that's correct wrt. its child nodes (i.e., the union of the child node deltas). If there are other ExtensionIds alongside TO_BE_INFERRED these will be kept (allowing a "lower bound" to be specified for individual nodes) * `Hugr::infer_extensions` also takes a `remove: bool` which, if true, modifies deltas of the same OpTypes that do *not* have TO_BE_INFERRED, by removing ExtensionIds. (Thus, non-inferred deltas act as upper bounds). BREAKING CHANGE: ExtensionError moved from hugr::validate to hugr; Hugr::infer_extensions takes bool parameter

## 🤖 New release * `hugr`: 0.5.1 -> 0.6.0 * `hugr-core`: 0.2.0 -> 0.3.0 * `hugr-passes`: 0.2.0 -> 0.3.0 * `hugr-cli`: 0.1.1 -> 0.1.2 <details><summary>Changelog</summary> ## `hugr` <blockquote> ## 0.6.0 (2024-06-28) ### Bug Fixes - SimpleReplacement panic on multiports ([#1191](#1191)) - Add some validation for const nodes ([#1222](#1222)) - Cfg not validating entry/exit types ([#1229](#1229)) - `extract_hugr` not removing root node ports ([#1239](#1239)) ### Documentation - Fix documentation of `ValidationError::ConstTypeError` ([#1227](#1227)) ### Features - CircuitBuilder::add_constant ([#1168](#1168)) - [**breaking**] Make the rewrite errors more useful ([#1174](#1174)) - [**breaking**] Validate Extensions using hierarchy, ignore input_extensions, RIP inference ([#1142](#1142)) - [**breaking**] Infer extension deltas for Case, Cfg, Conditional, DataflowBlock, Dfg, TailLoop ([#1195](#1195)) - Helper functions for requesting inference, use with builder in tests ([#1219](#1219)) ### Refactor - [**breaking**] FunctionBuilder takes impl Into<PolyFuncType> ([#1220](#1220)) - [**breaking**] Remove NodeType and input_extensions ([#1183](#1183)) </blockquote> ## `hugr-core` <blockquote> ## 0.3.0 (2024-06-28) ### Bug Fixes - SimpleReplacement panic on multiports ([#1191](#1191)) - Add some validation for const nodes ([#1222](#1222)) - Cfg not validating entry/exit types ([#1229](#1229)) - `extract_hugr` not removing root node ports ([#1239](#1239)) ### Documentation - Fix documentation of `ValidationError::ConstTypeError` ([#1227](#1227)) ### Features - CircuitBuilder::add_constant ([#1168](#1168)) - [**breaking**] Make the rewrite errors more useful ([#1174](#1174)) - [**breaking**] Validate Extensions using hierarchy, ignore input_extensions, RIP inference ([#1142](#1142)) - [**breaking**] Infer extension deltas for Case, Cfg, Conditional, DataflowBlock, Dfg, TailLoop ([#1195](#1195)) - Helper functions for requesting inference, use with builder in tests ([#1219](#1219)) ### Refactor - [**breaking**] Remove NodeType and input_extensions ([#1183](#1183)) - [**breaking**] FunctionBuilder takes impl Into<PolyFuncType> ([#1220](#1220)) </blockquote> ## `hugr-passes` <blockquote> ## 0.3.0 (2024-06-28) ### Features - [**breaking**] Validate Extensions using hierarchy, ignore input_extensions, RIP inference ([#1142](#1142)) - Helper functions for requesting inference, use with builder in tests ([#1219](#1219)) </blockquote> ## `hugr-cli` <blockquote> ## 0.1.1 (2024-06-07) ### Features - Reexport `clap::Parser` and `clap_verbosity_flag::Level` from hugr_cli ([#1146](#1146)) ### Refactor - Move binary to hugr-cli ([#1134](#1134)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). --------- Co-authored-by: Douglas Wilson <[email protected]>

acl-cqc added 2 commits June 12, 2024 15:31

BREAKING: Move ExtensionError into hugr-core/src/hugr/

03522c1

Add ExtensionSet::TO_BE_INFERRED, and check it's gone in validation

d134f88

acl-cqc changed the title ~~Infer extension deltas for Dfg, Cfg, etc.~~ feat: Infer extension deltas for Dfg, Cfg, etc. Jun 13, 2024

acl-cqc requested a review from ss2165 June 13, 2024 16:14

acl-cqc marked this pull request as ready for review June 13, 2024 16:14

acl-cqc requested a review from a team as a code owner June 13, 2024 16:14

acl-cqc commented Jun 13, 2024

View reviewed changes

acl-cqc changed the title ~~feat: Infer extension deltas for Dfg, Cfg, etc.~~ feat: Infer extension deltas for Dfg, Cfg, etc. + builder-RFC Jun 14, 2024

acl-cqc added 4 commits June 14, 2024 15:26

Inference algorithm

e835f56

If inferring, never remove

d63931d

Move TO_BE_INFERRED out of ExtensionSet into top-level of extension.rs

cd45bc9

inference tests

f3a05c4

acl-cqc force-pushed the acl-cqc/infer_deltas branch from 8cfbbd7 to 604b000 Compare June 14, 2024 14:39

ss2165 reviewed Jun 21, 2024

View reviewed changes

acl-cqc force-pushed the acl-cqc/infer_deltas branch from 604b000 to f3a05c4 Compare June 24, 2024 10:04

acl-cqc mentioned this pull request Jun 24, 2024

feat: Helper functions for requesting inference, use with builder in tests #1219

Merged

acl-cqc changed the title ~~feat: Infer extension deltas for Dfg, Cfg, etc. + builder-RFC~~ feat: Infer extension deltas for Dfg, Case, Cfg, Conditional, TailLoop Jun 24, 2024

acl-cqc changed the title ~~feat: Infer extension deltas for Dfg, Case, Cfg, Conditional, TailLoop~~ feat!: Infer extension deltas for Dfg, Case, Cfg, Conditional, TailLoop Jun 24, 2024

acl-cqc changed the title ~~feat!: Infer extension deltas for Dfg, Case, Cfg, Conditional, TailLoop~~ feat!: Infer extension deltas for Case, Cfg, Conditional, DataflowBlock, Dfg, TailLoop Jun 24, 2024

acl-cqc added 6 commits June 24, 2024 13:35

Make TO_BE_INFERRED an illegal ExtensionId

bf61478

Comment and message for ExtensionsNotInferred

25027f5

doc for infer_extensions, moving list of op types

0e90ed6

rename build_ext_(c=>d)fg

d36e34f

infer_three_generations: comment each case, and reorder

cd96d78

infer_three_generations: Check expected Hugr by backing up and mutating

6aa0193

acl-cqc requested a review from ss2165 June 24, 2024 13:32

ss2165 approved these changes Jun 24, 2024

View reviewed changes

Comment typo

4c5bf9c

acl-cqc enabled auto-merge June 24, 2024 16:08

acl-cqc added this pull request to the merge queue Jun 24, 2024

acl-cqc removed this pull request from the merge queue due to a manual request Jun 24, 2024

acl-cqc enabled auto-merge June 24, 2024 16:11

acl-cqc added this pull request to the merge queue Jun 24, 2024

Merged via the queue into main with commit 0b4a8b8 Jun 24, 2024
23 checks passed

acl-cqc deleted the acl-cqc/infer_deltas branch June 24, 2024 16:14

This was referenced Jun 24, 2024

chore: release #1181

Closed

chore: release #1233

Closed

This was referenced Jun 28, 2024

Infer extension-deltas for FuncDefn's #1237

Open

Infer extension-requirements for nested DFGs #640

Closed

hugrbot mentioned this pull request Jun 28, 2024

chore: release #1240

Merged

hugrbot mentioned this pull request Jul 1, 2024

chore: release #1248

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Infer extension deltas for Case, Cfg, Conditional, DataflowBlock, Dfg, TailLoop #1195

feat!: Infer extension deltas for Case, Cfg, Conditional, DataflowBlock, Dfg, TailLoop #1195

acl-cqc commented Jun 13, 2024 •

edited

Loading

codecov bot commented Jun 13, 2024 •

edited

Loading

acl-cqc Jun 13, 2024

ss2165 left a comment

ss2165 Jun 21, 2024

ss2165 Jun 21, 2024

acl-cqc Jun 24, 2024

ss2165 Jun 21, 2024

acl-cqc Jun 24, 2024

ss2165 Jun 21, 2024

acl-cqc Jun 24, 2024

ss2165 Jun 21, 2024

acl-cqc Jun 24, 2024

ss2165 Jun 21, 2024

acl-cqc Jun 24, 2024

ss2165 Jun 21, 2024

acl-cqc Jun 24, 2024

ss2165 Jun 21, 2024

acl-cqc Jun 24, 2024

acl-cqc commented Jun 24, 2024

ss2165 left a comment

ss2165 Jun 24, 2024

	/// A node claims to still be awaiting extension inference. Perhaps it is not acted upon by inference...
	/// A node claims to still be awaiting extension inference. Perhaps it is not acted upon by inference.

	fn build_ext_cfg(parent: ExtensionSet) -> (Hugr, Node) {
	fn build_ext_dfg(parent: ExtensionSet) -> (Hugr, Node) {

	// Base case failure: infers [XA] for parent but grandparent has disjoint et
	// Base case failure: infers [XA] for parent but grandparent has disjoint set

feat!: Infer extension deltas for Case, Cfg, Conditional, DataflowBlock, Dfg, TailLoop #1195

feat!: Infer extension deltas for Case, Cfg, Conditional, DataflowBlock, Dfg, TailLoop #1195

Conversation

acl-cqc commented Jun 13, 2024 • edited Loading

codecov bot commented Jun 13, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

ss2165 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc commented Jun 24, 2024

ss2165 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acl-cqc commented Jun 13, 2024 •

edited

Loading

codecov bot commented Jun 13, 2024 •

edited

Loading