fix!: Ops require their own extension #1226

acl-cqc · 2024-06-25T14:01:21Z

Includes adding new variants of block_builder, entry_builder, simple_entry_builder and conditional_builder: the default version omits the extension set parameter, the _exts variant takes an extra parameter (being an ExtensionSet). simple_block_builder is untouched (as it takes a FunctionType, so can use endo_ft/inout_ft)

We'll need similar updates to cfg_builder, tail_loop_builder, ConditionalBuilder::new and TailLoopBuilder::new but I'll leave those for another PR, there's quite enough here ;)

closes #388

BREAKING CHANGE: (1) container-node extension-deltas will need to be enlarged to include ops therein; for FuncDefn this will have to be manually specified but for other containers TO_BE_INFERRED, endo_ft or inout_ft all work. (2) block_builder, entry_builder, simple_entry_builder and conditional_builder no longer take an ExtensionSet; either drop the argument or use the ..._exts variant.

…features...

…o dfg)

codecov · 2024-06-28T09:55:19Z

Codecov Report

Attention: Patch coverage is 86.06557% with 34 lines in your changes missing coverage. Please review.

Project coverage is 87.09%. Comparing base (91f27f4) to head (aaa1cf3).
Report is 2 commits behind head on main.

Files	Patch %	Lines
hugr-passes/src/nest_cfgs.rs	0.00%	0 Missing and 12 partials ⚠️
hugr-core/src/builder/dataflow.rs	62.50%	0 Missing and 6 partials ⚠️
hugr-core/src/hugr/rewrite/simple_replace.rs	60.00%	0 Missing and 4 partials ⚠️
hugr-core/src/builder/cfg.rs	94.33%	0 Missing and 3 partials ⚠️
hugr-core/src/hugr/rewrite/replace.rs	71.42%	0 Missing and 2 partials ⚠️
hugr-passes/src/merge_bbs.rs	0.00%	0 Missing and 2 partials ⚠️
hugr-core/src/extension/op_def.rs	91.66%	0 Missing and 1 partial ⚠️
hugr-core/src/hugr/rewrite/outline_cfg.rs	0.00%	0 Missing and 1 partial ⚠️
hugr-core/src/hugr/serialize/test.rs	0.00%	0 Missing and 1 partial ⚠️
hugr-core/src/hugr/validate/test.rs	92.30%	0 Missing and 1 partial ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1226      +/-   ##
==========================================
- Coverage   87.18%   87.09%   -0.10%     
==========================================
  Files         107       99       -8     
  Lines       19538    18959     -579     
  Branches    17276    17302      +26     
==========================================
- Hits        17034    16512     -522     
+ Misses       1719     1663      -56     
+ Partials      785      784       -1

Flag	Coverage Δ
rust	`86.68% <86.06%> (+0.05%)`	⬆️

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.

aborgna-q

Nice! We can finally close one of the longest-standing issues.
I mostly have doc comments.

I agree with having the _exts suffix variants.
It's better to not muddle things with another trait that will still be set to None most of the time.
I'd just suggest swapping the extra argument to the end, to keep it consistent with the non-suffix ones.

f(*inputs, *outputs)
f_exts(*inputs, *outputs, extension_delta)

Also, it's not part of this PR but I think the ::builder::{ft1, ft2} helpers are quite confusing. They are used in doc examples but their names don't say anything (they rather look like local variables).

aborgna-q · 2024-07-04T08:27:40Z

hugr-core/src/builder/build_traits.rs

@@ -449,12 +470,12 @@ pub trait Dataflow: Container {
    ///
    /// This function will return an error if there is an error when building
    /// the Conditional node.
-    fn conditional_builder(
+    fn conditional_builder_exts(


Mention extension_delta in the fn docs.

aborgna-q · 2024-07-04T08:34:14Z

hugr-core/src/builder/cfg.rs

    /// Return a builder for the entry [`DataflowBlock`] child graph with `inputs`
    /// and `outputs` and the variants of the branching Sum value
    /// specified by `sum_rows`.


Suggested change

/// Return a builder for the entry [`DataflowBlock`] child graph with `inputs`

/// and `outputs` and the variants of the branching Sum value

/// specified by `sum_rows`.

/// Return a builder for the entry [`DataflowBlock`] child graph with

/// `outputs`, the variants of the branching Sum value

/// specified by `sum_rows`, and `extension_delta` explicitly specified.

Yes, pretty much did this, also noted that delta will be inferred on the implicit version

aborgna-q · 2024-07-04T08:36:40Z

hugr-core/src/builder/cfg.rs

+    /// Return a builder for the entry [`DataflowBlock`] child graph with `inputs`
+    /// and `outputs` and a UnitSum type: a Sum of `n_cases` unit types.


Suggested change

/// Return a builder for the entry [`DataflowBlock`] child graph with `inputs`

/// and `outputs` and a UnitSum type: a Sum of `n_cases` unit types.

/// Return a builder for the entry [`DataflowBlock`] child graph with

/// `outputs`, a Sum of `n_cases` unit types, and `extension_delta`

/// explicitly specified.

In fact I removed mention of non-existent parameter inputs from both, too. Plus cross-link.

In response to [this comment](#1226 (review)) that "the builder...helpers are quite confusing", this offers a better compromise between brevity and readability, i.e. slightly more of the latter. BREAKING CHANGE: builder::ft1 is now `builder::endo_ft`, similarly `ft2` is new `inout_ft`

acl-cqc · 2024-07-15T11:08:14Z

Description cleaned up - note

this is much less painful than the previous attempt fix: Ops require their own extensions #734 but there is still some pain for FuncDefn's (as there was always going to be) - that's Infer extension-deltas for FuncDefn's #1237.
Alternative considered but rejected - to keep one variant of each function, but take an exts: impl Into<Option<ExtensionSet>>. This would mean we'd write None for inference, rather than (in this PR) an extra _exts when we don't want inference.
- Using a custom trait (rather than Into<Option<ExtensionSet>>) would allow us to provide impl<T: Into<ExtensionSet>> OurExtensionSetTrait for T so we can use the useful Into<ExtensionSet>s that we have, alongside impl OurExtensionSetTrait for Option so we could still write None.
- Or we could just take exts: impl Into<ExtensionSet> and define some top-level const INFER of a type that impls Into<ExtensionSet> (producing ExtensionSet::singleton(TO_BE_INFERRED)). Then for inference you'd write INFER.
- We expect inference to be the normal hence I think the _exts is a good trade, writing nothing at all beats writing None or INFER

acl-cqc · 2024-07-15T11:09:24Z

Thanks @aborgna-q - merged with #1297 rename, added docs cross-links (I can take those out if you don't like them, but I think it's fine to put them in one way only, we want to encourage use of inference-by-default)

aborgna-q

Nice.
I don't want to get too caught up on the docs wording, so feel free to merge.

aborgna-q · 2024-07-15T13:19:51Z

hugr-core/src/builder/build_traits.rs

+    ///
+    /// The `other_inputs` must be an iterable over pairs of the type of the input and
+    /// the corresponding wire.
+    /// The `outputs` are the types of the outputs. Extension delta will be inferred.


Suggested change

/// The `outputs` are the types of the outputs. Extension delta will be inferred.

/// The `output_types` are the types of the outputs. Extension delta will be inferred.

aborgna-q · 2024-07-15T13:24:03Z

hugr-core/src/builder/build_traits.rs

    /// The `outputs` are the types of the outputs.
+    /// `exts` is the extension delta, here explicit. [conditional_builder](Self::conditional_builder) may be used to infer.


Using the same names as the parameters

Suggested change

/// The `outputs` are the types of the outputs.

/// `exts` is the extension delta, here explicit. [conditional_builder](Self::conditional_builder) may be used to infer.

/// The `output_types` are the types of the outputs.

/// `extension_delta` defines an explicit delta for the conditional. See [conditional_builder](Self::conditional_builder) may be used to infer it instead.

## 🤖 New release * `hugr`: 0.7.0 -> 0.8.0 * `hugr-core`: 0.4.0 -> 0.5.0 * `hugr-passes`: 0.4.0 -> 0.5.0 * `hugr-cli`: 0.1.3 -> 0.1.4 <details><summary>Changelog</summary> ## `hugr` <blockquote> ## 0.8.0 (2024-07-16) ### Bug Fixes - [**breaking**] Force_order failing on Const nodes, add arg to rank. ([#1300](#1300)) - NonConvex error on SiblingSubgraph::from_nodes with multiports ([#1295](#1295)) - [**breaking**] Ops require their own extension ([#1226](#1226)) ### Documentation - Attempt to correct force_order docs ([#1299](#1299)) ### Features - Make `DataflowOpTrait` public ([#1283](#1283)) - Make op members consistently public ([#1274](#1274)) ### Refactor - [**breaking**] Rename builder helpers: ft1->endo_ft, ft2->inout_ft ([#1297](#1297)) </blockquote> ## `hugr-core` <blockquote> ## 0.5.0 (2024-07-16) ### Bug Fixes - NonConvex error on SiblingSubgraph::from_nodes with multiports ([#1295](#1295)) - [**breaking**] Ops require their own extension ([#1226](#1226)) ### Features - Make `DataflowOpTrait` public ([#1283](#1283)) - Make op members consistently public ([#1274](#1274)) ### Refactor - [**breaking**] Rename builder helpers: ft1->endo_ft, ft2->inout_ft ([#1297](#1297)) </blockquote> ## `hugr-passes` <blockquote> ## 0.5.0 (2024-07-16) ### Bug Fixes - [**breaking**] Ops require their own extension ([#1226](#1226)) - [**breaking**] Force_order failing on Const nodes, add arg to rank. ([#1300](#1300)) ### Documentation - Attempt to correct force_order docs ([#1299](#1299)) ### Refactor - [**breaking**] Rename builder helpers: ft1->endo_ft, ft2->inout_ft ([#1297](#1297)) </blockquote> ## `hugr-cli` <blockquote> ## 0.1.3 (2024-07-10) ### Styling - Change "serialise" etc to "serialize" etc. ([#1251](#1251)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/).

closes #1318 and also deals with BlockBuilder not mentioned there. I think this is now all of the nested-structures covered: | | XBuilder::new | fn x | |----|----|----| |Conditional|**here**|in #1226| |\->Case|takes `Signature`|inherits exts| |TailLoop|**here**|**here**| |DFG|takes `Signature`|takes `Signature` (`dfg_builder_endo` in #1219)| |CFG|takes `Signature`|**here**| |\->Block|**here**|in #1226| (FuncDefn takes `Signature` and is *not supported by inference yet* anyway) BREAKING CHANGE: `cfg_builder`, `tail_loop_builder`, `ConditionalBuilder::new`, `BlockBuilder::new` and `TailLoopBuilder::new` no longer take an ExtensionSet parameter; either remove the argument (to use extension inference) or use the `_exts` variant

acl-cqc force-pushed the acl/builder_infer branch from 0e6fd1e to d4d1c4e Compare June 25, 2024 17:14

acl-cqc force-pushed the acl/ops_require_ext branch 2 times, most recently from c0d6432 to ce6638f Compare June 25, 2024 18:10

Base automatically changed from acl/builder_infer to main June 28, 2024 08:27

acl-cqc added 16 commits June 28, 2024 10:43

Insert OpDef extension in compute_signature

7832cf0

fix tests w/out extension_inference (only 3). 42 failures with --all-…

d6802c3

…features...

simple_replace/sibling_subgraph - 23 failures remaining

2c32b1a

validate, note about closed_dfg_root_hugr

5cb15ca

extension (op_def.rs, prelude.rs), serialize

5b05df3

replace.rs - need better CFG support here

78f7aed

builder copy_insertion+nested_identity (change from module+function t…

856bfd4

…o dfg)

circuit (explicit Function)

b2c09d5

views (hugr-core now ok, and so, onto hugr-passes)

10bab5d

const_fold (WIN) - int_fn+float_fn+others => noargfn calling ft2

9e47b98

merge_bbs (TODO simple_block_builder, block_builder)

290bb6a

doctests

eca016d

merge_bbs use simple_entry_builder

abbfa2a

(simple_)block_builder variants, fix tests

3994ee9

Conditional builder (_exts)

0da41d8

benches

a238f21

acl-cqc force-pushed the acl/ops_require_ext branch from ce6638f to a238f21 Compare June 28, 2024 09:43

clippy

79da288

This was referenced Jun 28, 2024

Infer extension-requirements for nested DFGs #640

Closed

Tracking issue for extension requirement inference #426

Open

acl-cqc marked this pull request as ready for review June 28, 2024 10:09

acl-cqc requested a review from a team as a code owner June 28, 2024 10:09

acl-cqc requested a review from zrho June 28, 2024 10:09

acl-cqc mentioned this pull request Jun 28, 2024

fix: Ops require their own extensions #734

Closed

acl-cqc requested a review from aborgna-q July 3, 2024 09:29

acl-cqc removed the request for review from zrho July 3, 2024 09:29

aborgna-q reviewed Jul 4, 2024

View reviewed changes

acl-cqc mentioned this pull request Jul 8, 2024

RFC: Replace ft1/ft2 with Into<FunctionType> from tuple of TypeRow(s) #1269

Closed

acl-cqc mentioned this pull request Jul 15, 2024

refactor!: Rename builder helpers: ft1->endo_ft, ft2->inout_ft #1297

Merged

acl-cqc added 2 commits July 15, 2024 11:53

Merge remote-tracking branch 'origin/main' into ops_require_ext

ab4ef40

doc improvements

1b5eebd

acl-cqc requested a review from aborgna-q July 15, 2024 11:08

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

fix sibling_subgraph

eb6d1d0

force_order

a574c6d

aborgna-q approved these changes Jul 15, 2024

View reviewed changes

acl-cqc added 2 commits July 15, 2024 15:09

docs: correct output_types parameter name

aaa1cf3

extension_delta last param everytime

2c56bf6

acl-cqc enabled auto-merge July 15, 2024 14:19

acl-cqc added this pull request to the merge queue Jul 15, 2024

Merged via the queue into main with commit cfb0674 Jul 15, 2024
18 checks passed

acl-cqc deleted the acl/ops_require_ext branch July 15, 2024 14:23

hugrbot mentioned this pull request Jul 15, 2024

chore: release #1296

Closed

ss2165 mentioned this pull request Jul 15, 2024

Python tests failing #1301

Closed

hugrbot mentioned this pull request Jul 15, 2024

chore: release #1305

Merged

hugrbot mentioned this pull request Jul 16, 2024

chore: release #1314

Merged

This was referenced Jul 16, 2024

resolve_opaque_op fails when OpaqueOp does not explicitly require it's own extension. #1315

Closed

fix: add op's extension to signature check in resolve_opaque_op #1317

Merged

Remaining bits of builder frontend for inference #1318

Closed

acl-cqc mentioned this pull request Aug 1, 2024

feat!: Update remaining builder methods to "infer by default" #1386

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!: Ops require their own extension #1226

fix!: Ops require their own extension #1226

acl-cqc commented Jun 25, 2024 •

edited

Loading

codecov bot commented Jun 28, 2024 •

edited

Loading

aborgna-q left a comment •

edited

Loading

aborgna-q Jul 4, 2024

aborgna-q Jul 4, 2024

acl-cqc Jul 15, 2024

aborgna-q Jul 4, 2024

acl-cqc Jul 15, 2024

acl-cqc commented Jul 15, 2024

acl-cqc commented Jul 15, 2024

aborgna-q left a comment •

edited

Loading

aborgna-q Jul 15, 2024

aborgna-q Jul 15, 2024

acl-cqc Jul 15, 2024

		/// Return a builder for the entry [`DataflowBlock`] child graph with `inputs`
		/// and `outputs` and a UnitSum type: a Sum of `n_cases` unit types.

	/// The `outputs` are the types of the outputs. Extension delta will be inferred.
	/// The `output_types` are the types of the outputs. Extension delta will be inferred.

		/// The `outputs` are the types of the outputs.
		/// `exts` is the extension delta, here explicit. [conditional_builder](Self::conditional_builder) may be used to infer.

fix!: Ops require their own extension #1226

fix!: Ops require their own extension #1226

Conversation

acl-cqc commented Jun 25, 2024 • edited Loading

codecov bot commented Jun 28, 2024 • edited Loading

Codecov Report

aborgna-q left a comment • edited Loading

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 Jul 15, 2024

acl-cqc commented Jul 15, 2024

aborgna-q left a comment • edited Loading

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 25, 2024 •

edited

Loading

codecov bot commented Jun 28, 2024 •

edited

Loading

aborgna-q left a comment •

edited

Loading

aborgna-q left a comment •

edited

Loading