-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feature] Allow @guidance
to decorate methods
#1035
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files@@ Coverage Diff @@
## main #1035 +/- ##
==========================================
- Coverage 70.25% 61.94% -8.32%
==========================================
Files 62 62
Lines 4472 4533 +61
==========================================
- Hits 3142 2808 -334
- Misses 1330 1725 +395 ☔ View full report in Codecov by Sentry. |
I'm unsatisfied with the behavior of caches here -- they will prevent instances from being garbage collected, and cache entries themselves won't get cleared. Need to tinker with that a bit more... |
Maybe use weakref? The cached objects can then live and be used for a while, but will be cleared during a garbage collection if there is too much memory pressure. |
My thoughts exactly. Woke up this morning in a cold sweat mumbling to myself "...the garbage collector!" (joking). Notice that there are essentially two levels of caches here -- one on the decorated function itself which is needed for "grammar node reuse" that wraps the unbound function (note it will receive the My thought is to essentially use a weakref for the latter cache and then move the other cache to the bound method so that it doesn't need to hold a reference to the instance at all. Make general sense? |
Side note, but I'm getting uncomfortable with the fact that we're not setting |
Non-side note: @paulbkoch how do you feel about the spirit of this PR in the first place? Do you think that allowing methods to be decorated is a good thing? 😄 |
…e with other tests before)
@guidance
to decorate methods@guidance
to decorate methods
@paulbkoch I finally feel good about the caching. Feel free to take a look on your own time or I'd be happy to jump on a call and do some code review 😄 |
Had a think on this -- I'm a fan of the concept. Python decorators are allowed to be used everywhere, so it seems quite reasonable to me to make the guidance decorator work on class methods too. I think there is organizational/bookkeeping utility from being able to do this in our own codebase, even if end users don't take advantage of it as much. |
@Harsha-Nori glad you're a fan of the idea. Before pushing this through, I want to have a deeper conversation about what is or isn't cached, especially for the sake of recursive calls. Some subtle things going on there, even for vanilla guidance functions (non-methods). Definitely don't want to be adding unnecessary cognitive load for users. |
This PR extends the
@guidance
decorator to work with methods.I believe this will be invaluable when defining grammars that are implemented as several guidance functions that call each other, particularly if they need to share common parameters. I think that rewriting
json
as a class will be a good test case for this, and I expect to be able to simplify the implementation with this new machinery as well as enable some otherwise-tricky shared parameters likeseparators=(", ", ": ")
.To achieve this:
GuidanceFunction
type that is returned when the decorator is called on a function.GuidanceFunction
uses__get__
descriptor magic to ensure that the function is bound to an instance before being wrapped by the decorator's actual implementation (_decorator
).GuidanceFunction
s are given aGuidanceMethod
typeSide note/todo:
_guidance.pyi
is now not strictly correct, but it still gives helpful type hints. It's most-wrong in the case where a user guidance-decorates a method but tries to call it from the class rather than an instance. This case should be broken anyway, but I need to think some more about how to give the user a good error message. IMO, that can wait for a follow-up PR.