Convert Template into an operation
#424
Conversation
* install prettyprinter for term when library is available * lint * move code into types.py
* convert evaluate to a singledispatch * lint
* deprecate defterm * remove defterm case * remove defterm * lint * evaluate distribution arguments * lint * remove interpreter * Revert "remove interpreter" This reverts commit 3044277. * wip * lint
… and handling of distribution methods (#311) * refactor distribution operations * add a test for typeof of distributions * add tests for symbolic dists/arguments * introduce operations for distribution methods * comment * fix tests * work around #310 * replace hack with new hack * tweak repr for _BaseOperation * lint * work around #312 * clean up access to dist ops * wip * wip * add type annotations to get correct term conversion * lint * include distribution arguments as properties * fix distribution calls * try again * fixes * format
* box the output of __type_rule__ * fix tests * fix tests * require callers of __type_rule__ to box arguments * fix * move Box out of ops.types * lint * fix test
* fix recursion error in sizesof * format
eb8680
left a comment
eb8680
left a comment
There was a problem hiding this comment.
A few more nits, but are you planning to address my previous comment about Template.__get__ here or do you think that's out of scope for this PR? I do want to get this merged soon but on the other hand Template.__get__ is going to become more incorrect than it currently is in staging-llm if it just directly inherits Operation.__get__.
effectful/handlers/llm/template.py
Outdated
|
|
||
| for param_name, param in signature.parameters.items(): | ||
| # allow self to be unannotated | ||
| if param_name != "self" and param.annotation is inspect.Parameter.empty: |
There was a problem hiding this comment.
Matching on the literal "self" to detect that this is a descriptor is not sound, this should be fixed or removed.
There was a problem hiding this comment.
Addressing Template.__get__ might actually make doing the right thing here easier, so I think that's worth a try.
tests/test_handlers_llm_template.py
Outdated
|
|
||
| @Template.define | ||
| def f(self) -> int: | ||
| """What is the number after {self.x}?""" |
There was a problem hiding this comment.
I'm prettys ure this self.x reference will not behave as expected under a Template.__apply__ handler in the current implementation. We could try to fix this behavior, or we could disallow this for the moment, add an xfailing test and address it in a followup PR.
There was a problem hiding this comment.
One interesting possibility is to use the lexical context as a source of template variables. This would make the behavior more like an f-string. I've added some tests to demonstrate this behavior.
Replaces the
Template.__call__operation withTemplate.apply. Blocked by #423