feat!: Allow static inputs to extension operations #1628
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
BREAKING CHANGE: `PolyFuncTypeRV` replaced with new type `OpDefSignature` that also holds static input types.
+ static port counting trait methods
ss2165
force-pushed
the
ss/ext-static-in
branch
from
c078eab
to
4f99254
Compare
|
This PR contains breaking changes to the public Rust API. cargo-semver-checks summary |
| let op: OpType = nodetype.into(); | ||
| let static_in_ports = op.static_input_ports(); | ||
| let handle = self.add_dataflow_op(op, input_wires)?; | ||
|
|
||
| for (src, in_port) in static_wires.into_iter().zip(static_in_ports) { | ||
| self.hugr_mut() | ||
| .connect(src.node(), src.source(), handle.node(), in_port); | ||
| } | ||
| Ok(handle) |
There was a problem hiding this comment.
Can we use self.add_node_with_wires with chained input_ and static_ directly?
| /// Returns the function type of the signature. | ||
| pub fn func_type(&self) -> &Signature { | ||
| &self.func_type | ||
| } | ||
|
|
||
| /// Returns the static inputs of the signature. | ||
| pub fn static_inputs(&self) -> &TypeRow { | ||
| &self.static_inputs | ||
| } |
There was a problem hiding this comment.
Since we have these already we could make the attributes private.
Or do we need mut access?
| #[derive(Clone, Debug, PartialEq, Eq, Hash)] | ||
| #[cfg_attr(test, derive(Arbitrary), proptest(params = "RecursionDepth"))] | ||
| pub struct ExtOpSignature { | ||
| // #[serde(flatten)] |
There was a problem hiding this comment.
| // #[serde(flatten)] |
?
| @@ -47,9 +54,93 @@ pub type PolyFuncType = PolyFuncTypeBase<NoRV>; | |||
| /// The polymorphic type of an [OpDef], whose number of input and outputs | |||
| /// may vary according to how [RowVariable]s therein are instantiated. | |||
| /// | |||
| /// It may also have a row of static inputs. | |||
There was a problem hiding this comment.
Do we state anywhere the order between static and runtime inputs in a node?
| pub type PolyFuncTypeRV = PolyFuncTypeBase<RowVariable>; | ||
| #[derive(Clone, PartialEq, Debug, Default, Eq, Hash, derive_more::Display)] | ||
| #[cfg_attr(test, derive(Arbitrary), proptest(params = "RecursionDepth"))] | ||
| #[display("{}{}{}", self.display_params(), self.static_inputs, self.body())] |
There was a problem hiding this comment.
ExtOpSignature displays the static_inputs between <>, if non-emtpy.
Should we do the same here?
There was a problem hiding this comment.
I wonder if OpDefSignature should now move into hugr_core::extension
| if let [p] = self.static_ports(Direction::Outgoing).as_slice() { | ||
| Some(p.as_outgoing().unwrap()) | ||
| } else { | ||
| None | ||
| } | ||
| // .map(|p| p.as_outgoing().unwrap()).collect() |
There was a problem hiding this comment.
| if let [p] = self.static_ports(Direction::Outgoing).as_slice() { | |
| Some(p.as_outgoing().unwrap()) | |
| } else { | |
| None | |
| } | |
| // .map(|p| p.as_outgoing().unwrap()).collect() | |
| match self.static_ports(Direction::Outgoing).as_slice() { | |
| [p] => Some(p.as_outgoing().unwrap()), | |
| _ => None, | |
| } |
There was a problem hiding this comment.
Many of the OpType methods look like they could return an impl Iterator instead of collecting a vec.
Are there any lifetime problems with that?
| } | ||
| } | ||
|
|
||
| /// Returns the [`ExtOpSignature`] of this [`ExtensionOp`]. |
There was a problem hiding this comment.
nit: We can already see the types.
| /// Returns the [`ExtOpSignature`] of this [`ExtensionOp`]. | |
| /// Return the instantiated signature of this [`ExtensionOp`]. |
| def add( | ||
| self, | ||
| com: ops.Command, | ||
| *, | ||
| static_in: Iterable[Wire] | None = None, | ||
| metadata: dict[str, Any] | None = None, | ||
| ) -> Node: |
There was a problem hiding this comment.
Add the static_in argument to add_op too (and maybe to _wire_up), and use it here.
| self, | ||
| com: Command, | ||
| *, | ||
| static_in: Iterable[Wire] | None = None, |
There was a problem hiding this comment.
The parameter is not used?
|
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 |
We do not allow
|
|
....and specifically, Now, IIUC there are proposals to allow a |
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 #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? |
I don't think so. This PR introduces an additional parameter list to operations, so that operations can have three different types of inputs:
And this does not appear to be an implementation detail, but part of the specification of an operation. |
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. |
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 |
Co-authored-by: Alan Lawrence <[email protected]>
I'm not sure I understand here - PR leaves the signature of a node unchanged, as a |
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 |
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. |
| } | ||
| } | ||
|
|
||
| /// Instantiated [OpDef] signature. |
There was a problem hiding this comment.
Suggest this goes in hugr_core::ops::custom
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, DataflowOpTraitmethods which assumed one static input now support many. E.g.static_input_port->static_input_ports. Returning aVecrather than anOption. Operation definitions support static input type declarations, so a new typeOpDefSignaturehas been introduced for this to hold thePolyFuncTypeRVand the static inputs. The cached signature of an ExtensionOp is also a new typeExtOpSignaturethat holds the dataflowSignatureand the static inputs.OpTag::StaticInputremoved because it doesn't actually narrow things down any more.