feat: Decorator to auto lower to hugr extension #1091
Conversation
|
Blocked by CQCL/hugr#2520. |
There was a problem hiding this comment.
Thanks @hsemenenko - sorry for the slow response - looks good and I am 90% of the way to hitting Approve ;). A couple of thoughts....
and the big issue may be, do we know that setting a custom-lowering-to-Hugr actually achieves anything? (That is, we may also need work on the Hugr/TKET2 side, separately from this PR.)
guppylang-internals/src/guppylang_internals/definition/lowerable.py
Outdated
Show resolved
Hide resolved
| ], | ||
| ) | ||
|
|
||
| hugr_ext.add_op_def(op_def) |
There was a problem hiding this comment.
I'm a bit nervous here - if we have multiple uses of the same (decorated) function, do we call add_op_def multiple times? Or does this happen only once, when python encounters the @lowered_op? If the latter, what happens if I have two different but same-named @lowered_ops? (We would want that to be an error, methinks)
There was a problem hiding this comment.
Good observation. In the case where the function was called multiple times the second usage would override the first. I have moved the op definition outside this function so that it is only called a single time.
guppylang-internals/src/guppylang_internals/definition/lowerable.py
Outdated
Show resolved
Hide resolved
| code. The only information we need to access is that it's a function type and | ||
| that there are no unsolved existential vars. | ||
| """ | ||
| from guppylang_internals.definition.function import parse_py_func |
There was a problem hiding this comment.
Hmmmm, ok. So the differences against the RawCustomFunctionDef.parse that we're overriding here are:
- we suppress the error if there is a body
- we don't look at
self.signaturebefore we callself._get_signature(func_ast, globals)....is that right? - we don't assign the local variable
docstringthat AFAICS in the original/overridden method is defined but not used(?!)
There is definitely an opportunity here for both commoning-up, and possibly cleaning up....do you feel like taking it?
There was a problem hiding this comment.
Yes, these are the difference between the two methods. As suppressing the error if there is no body is required for the lowering case, it was suggested that these implementations were kept separate. This does come with a small side affect that self.signature is not used.
There was a problem hiding this comment.
Yes, agreed that you need suppress the warning, and since you are now using self.signature, looks good to me. You could possibly add a private helper that does the sig = .....; ty = .....; return CustomFunctionDef(self...., self....) etc. (taking just self and func_ast as parameters) but fine as it stands.
… to hugr extension
|
Thanks @acl-cqc I've addressed your comments. As far as I am aware, |
There was a problem hiding this comment.
Thanks @hsemenenko, sorry I missed the request for rereview! Looks good to me....the only thing I can see that might be worth adding would be a test that if you define two functions with the same name, both with @lowerable_op(the_same_ext), that the second either errors, or properly replaces the first (without inheriting the lower-function, say), as appropriate.
| code. The only information we need to access is that it's a function type and | ||
| that there are no unsolved existential vars. | ||
| """ | ||
| from guppylang_internals.definition.function import parse_py_func |
There was a problem hiding this comment.
Yes, agreed that you need suppress the warning, and since you are now using self.signature, looks good to me. You could possibly add a private helper that does the sig = .....; ty = .....; return CustomFunctionDef(self...., self....) etc. (taking just self and func_ast as parameters) but fine as it stands.
Closes #797