feat!: Reject early bad calls to OpDef.instantiate - #3200
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3200 +/- ##
=======================================
Coverage 81.45% 81.45%
=======================================
Files 243 243
Lines 46953 46967 +14
Branches 40702 40702
=======================================
+ Hits 38245 38259 +14
Misses 6706 6706
Partials 2002 2002
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| concrete_signature: Concrete function type of the operation, only required | ||
| if the operation is polymorphic. | ||
| """ |
There was a problem hiding this comment.
Add a Raises section to the docstring
| f"Too many args: Op {self.name} takes {len(params)} args, " | ||
| f"but was given {num_args}" | ||
| ) | ||
| raise TypeError(msg) |
There was a problem hiding this comment.
These should rather be ValueErrors, the types are correct.
| params = self.signature.poly_func.params | ||
| if num_args > len(params): | ||
| msg = ( | ||
| f"Too many args: Op {self.name} takes {len(params)} args, " |
There was a problem hiding this comment.
Use self.qualified_name(), so the op is clearly disambiguated.
There was a problem hiding this comment.
Do we need to make this a breaking change?
Cases that would error out now produced invalid hugrs before, so we are only breaking invalid uses 🤔
There was a problem hiding this comment.
Before this PR, we are successfully making the hugrs, but with unsolved type variables. I remember that at some point, when parsing the model we wanted to be able to silently solve these variables when possible, so I'm not sure if we can say the hugrs are invalid?
My thinking about the breakage is that people could have programs generating hugrs that they haven't validated, which would now surprisingly break
2 participants
OpDef.instantiaterequires a concrete signature to be provided in order to add a polymorphic operation to the graph. Otherwise, types in the operation's signature will be left out, meaning the resulting hugr wont validate, as in #3189.Here, I try to raise this error earlier, by checking if the concrete signature is necessary, as well as raising errors for an incorrect number of type args.
Aside
The signature of instantiate:
should really be
since the
FunctionTypeneeds to be provided whenever any type args are given. But this isn't usually done in python, so I thought I'd get pushback for the ugly APIBREAKING CHANGE: Raise errors in bad calls to
OpDef.instantiate