feat!: Allow static inputs to extension operations

ss2165 commented 3 weeks ago

Extension operations can now have some number of static inputs that come between their dataflow input ports and the state order input port.

This can be used for compile-time-known inputs to operations. I have attempted to keep the serialization backwards compatible.

OpDefs can either declare their static inputs as a row, or it can be calculated like the rest of the signature.

Closes #1594

BREAKING CHANGE: OpType, OpTrait, DataflowOpTrait methods which assumed one static input now support many. E.g. static_input_port -> static_input_ports. Returning a Vec rather than an Option. Operation definitions support static input type declarations, so a new type OpDefSignature has been introduced for this to hold the PolyFuncTypeRV and the static inputs. The cached signature of an ExtensionOp is also a new type ExtOpSignature that holds the dataflow Signature and the static inputs. OpTag::StaticInput removed because it doesn't actually narrow things down any more.

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 90.93484% with 32 lines in your changes missing coverage. Please review.

Project coverage is 85.82%. Comparing base (9962f97) to head (6b3085d). Report is 16 commits behind head on main.

Files with missing lines	Patch %	Lines
hugr-core/src/ops/custom.rs	79.41%	14 Missing :warning:
hugr-core/src/extension/op_def.rs	86.74%	7 Missing and 4 partials :warning:
...e/src/extension/op_def/serialize_signature_func.rs	92.85%	1 Missing and 1 partial :warning:
hugr-core/src/types/poly_func.rs	95.34%	2 Missing :warning:
hugr-core/src/builder/build_traits.rs	93.33%	0 Missing and 1 partial :warning:
hugr-core/src/export.rs	50.00%	0 Missing and 1 partial :warning:
hugr-core/src/ops.rs	96.96%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1628 +/- ## ========================================== + Coverage 85.78% 85.82% +0.04% ========================================== Files 136 136 Lines 25135 25403 +268 Branches 22061 22320 +259 ========================================== + Hits 21561 21803 +242 - Misses 2425 2446 +21 - Partials 1149 1154 +5 ``` | [Flag](https://app.codecov.io/gh/CQCL/hugr/pull/1628/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CQCL) | Coverage Δ | | |---|---|---| | [python](https://app.codecov.io/gh/CQCL/hugr/pull/1628/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CQCL) | `92.47% <100.00%> (+0.02%)` | :arrow_up: | | [rust](https://app.codecov.io/gh/CQCL/hugr/pull/1628/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CQCL) | `84.91% <90.53%> (+0.05%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CQCL#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

hugrbot commented 3 weeks ago

This PR contains breaking changes to the public Rust API.

cargo-semver-checks summary

``` --- failure enum_no_repr_variant_discriminant_changed: enum variant had its discriminant change value --- Description: The enum's variant had its discriminant value change. This breaks downstream code that used its value via a numeric cast like `as isize`. ref: https://doc.rust-lang.org/reference/items/enumerations.html#assigning-discriminant-values impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/enum_no_repr_variant_discriminant_changed.ron Failed in: variant OpTag::StaticOutput 16 -> 15 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:50 variant OpTag::FnCall 17 -> 16 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:52 variant OpTag::LoadConst 18 -> 17 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:54 variant OpTag::LoadFunc 19 -> 18 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:56 variant OpTag::ScopedDefn 20 -> 19 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:58 variant OpTag::TailLoop 21 -> 20 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:60 variant OpTag::Conditional 22 -> 21 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:62 variant OpTag::Case 23 -> 22 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:64 variant OpTag::Leaf 24 -> 23 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:66 variant OpTag::DataflowBlock 25 -> 24 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:69 variant OpTag::BasicBlockExit 26 -> 25 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:71 variant OpTag::StaticOutput 16 -> 15 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:50 variant OpTag::FnCall 17 -> 16 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:52 variant OpTag::LoadConst 18 -> 17 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:54 variant OpTag::LoadFunc 19 -> 18 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:56 variant OpTag::ScopedDefn 20 -> 19 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:58 variant OpTag::TailLoop 21 -> 20 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:60 variant OpTag::Conditional 22 -> 21 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:62 variant OpTag::Case 23 -> 22 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:64 variant OpTag::Leaf 24 -> 23 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:66 variant OpTag::DataflowBlock 25 -> 24 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:69 variant OpTag::BasicBlockExit 26 -> 25 in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/tag.rs:71 --- failure enum_variant_missing: pub enum variant removed or renamed --- Description: A publicly-visible enum has at least one variant that is no longer available under its prior name. It may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/enum_variant_missing.ron Failed in: variant OpTag::StaticInput, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/ops/tag.rs:50 variant OpTag::StaticInput, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/ops/tag.rs:50 --- failure inherent_method_missing: pub method removed or renamed --- Description: A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/inherent_method_missing.ron Failed in: OpType::static_port, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/ops.rs:232 OpType::static_input_port, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/ops.rs:240 OpType::static_port, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/ops.rs:232 OpType::static_input_port, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/ops.rs:240 --- failure method_parameter_count_changed: pub method parameter count changed --- Description: A publicly-visible method now takes a different number of parameters. ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/method_parameter_count_changed.ron Failed in: hugr_core::ops::custom::OpaqueOp::new now takes 6 parameters instead of 5, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/custom.rs:223 hugr_core::ops::OpaqueOp::new now takes 6 parameters instead of 5, in /home/runner/work/hugr/hugr/PR_BRANCH/hugr-core/src/ops/custom.rs:223 --- failure trait_method_missing: pub trait method removed or renamed --- Description: A trait method is no longer callable, and may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#major-any-change-to-trait-item-signatures impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/trait_method_missing.ron Failed in: method static_input of trait DataflowOpTrait, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/ops/dataflow.rs:49 method static_input of trait DataflowOpTrait, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/ops/dataflow.rs:49 method static_source of trait HugrView, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/views.rs:420 method static_source of trait HugrView, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/views.rs:420 method static_source of trait HugrView, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/hugr/views.rs:420 method static_input of trait OpTrait, previously in file /home/runner/work/hugr/hugr/BASELINE_BRANCH/hugr-core/src/ops.rs:387 ```

zrho commented 2 weeks ago

I'm wondering about the purpose of static inputs. So these are statically known runtime values, which are statically known because they are produced by a const node? We already have a mechanism to provide statically known information to an operation via TypeArgs; this isn't a hack but appears to be the most natural way to do this anyway. In that light I'd advocate for removing static inputs entirely instead of allowing multiple of them. Am I missing something?

doug-q commented 2 weeks ago

I'm wondering about the purpose of static inputs. So these are statically known runtime values, which are statically known because they are produced by a const node? We already have a mechanism to provide statically known information to an operation via TypeArgs; this isn't a hack but appears to be the most natural way to do this anyway. In that light I'd advocate for removing static inputs entirely instead of allowing multiple of them. Am I missing something?

We do not allow hugr::ops::constant::Value inside TypeArgs because Value does not have ==. Allowing Value inside TypeArg would mean that Type would not have ==, which is untenable.

Value cannot be given == because:

It might be a Value::Function, i.e. an embedded HUGR. A Hugr can't have == because Value can't, and because equality of programs is problematic.
It might be a CustomConst which does not support equality.

CustomConst cannot be given == because:

It might be a float, where equality does not exactly make sense(NAN).
It might be a user CustomConst where not enough information is available to determine equality. e.g. two symbol names that are not the same, but might be linked together later on.

acl-cqc commented 2 weeks ago

....and specifically, TypeArg does not allow to contain static functions.

Now, IIUC there are proposals to allow a TypeArg:Node, which if it referenced a funcdefn, would allow a "loadconst"-like op to produce a function value (the TypeArg replacing the Const, or indeed, you could say it was a "loadfunction"-like op) - and then removing the ability to have Hugr's nested inside consts (==> equivalent to saying static function values can only be produced by LoadFunction). I haven't thought through monomorphic-vs-polymorphic, typeapply, etc. yet tho

acl-cqc commented 2 weeks ago

In that light I'd advocate for removing static inputs entirely instead of allowing multiple of them

In some sense, this could well be where we are headed - but we certainly wouldn't want to remove them until we have finalized+implemented https://github.com/CQCL/hugr/discussions/1425! So in the meantime, aren't "static inputs" just the hugr-core way of representing (with nodes and edges) the noderef-like "terms" proposed in that discussion?

zrho commented 2 weeks ago

So in the meantime, aren't "static inputs" just the hugr-core way of representing (with nodes and edges) the noderef-like "terms" proposed in that discussion?

I don't think so. This PR introduces an additional parameter list to operations, so that operations can have three different types of inputs:

static values (TypeArg in core, Term in model)
runtime values, but statically known
runtime values

And this does not appear to be an implementation detail, but part of the specification of an operation.

acl-cqc commented 2 weeks ago

operations can have three different types of inputs:

static values (TypeArg in core, Term in model)

runtime values, but statically known

runtime values

I thought the idea of #1425 was that the first two of these would be Terms in the model, which would include all "constant values" (probably excluding nested Hugrs, instead allowing references to function nodes). That some of these Terms would (at least initially) be turned into nodes with edges and others would be left as TypeArgs is an implementation detail of hugr-core and not part of the model.

zrho commented 2 weeks ago

That some of these Terms would (at least initially) be turned into nodes with edges and others would be left as TypeArgs is an implementation detail of hugr-core and not part of the model.

Yes, in the standup where we gave the thumbs up for this I understood it in that way. But that appears not compatible with how this PR does it since it makes static inputs into their own part of the signature. It therefore can't be a hugr-core implementation detail.

acl-cqc commented 2 weeks ago

That some of these Terms would (at least initially) be turned into nodes with edges and others would be left as TypeArgs is an implementation detail of hugr-core and not part of the model.

But that appears not compatible with how this PR does it since it makes static inputs into their own part of the signature. It therefore can't be a hugr-core implementation detail.

I'm not sure I understand here - PR leaves the signature of a node unchanged, as a pub type Signature = FuncTypeBase<NoRV>. An OpDef now has a type_row of static inputs as well, i.e. an OpDefSignature rather than a PolyFuncTypeBase<RowVariable> - is that the problem, i.e. in the serialization==model-representation of extension definitions ?

ss2165 commented 2 weeks ago

PR leaves the signature of a node unchanged,

Don't think this is true it may not be stored in the dataflow signature part but nodes still have to report their static signature, like using ExtOpSignature for extension ops.

acl-cqc commented 2 weeks ago

Don't think this is true it may not be stored in the dataflow signature part but nodes still have to report their static signature, like using ExtOpSignature for extension ops.

Ah I see. Yes if that gets written out in the model then that doesn't sound like what we want. That info will eventually go in the model in the Term (equiv to TypeArg) part, IIUC.

ss2165 commented 6 days ago

Closing as stale, subsumed by work in #1433

CQCL / hugr

feat!: Allow static inputs to extension operations #1628

Codecov Report