-
Notifications
You must be signed in to change notification settings - Fork 53
feat: add async functions #169
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
Conversation
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
|
Looks good. Couple of thoughts:
|
We currently don't have a great way to do this. In the initial proposal, we talked about attaching a logging callback for these things but it hasn't been implemented yet. Let's talk about this in the mellea meeting today. |
|
Added async generative slots. Note, the async nature of the functions don't interfere with the schema generation: In other words, the function doesn't have some wonky return type like |
|
After chatting with Nathan, removed references to sessions' backend stacks since we aren't using them. |
bb4740e to
ace40b7
Compare
d817cac to
c956d8e
Compare
avinash2692
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Added top-level async functions and async functions to MelleaSessions. Also added a short section to the tutorial.
This PR does not support async generative slots; I have it working, but I am trying to find a way for type checkers to be happy.This PR supports async generative slots with a happy type checker.There is some code duplication between sync and asynchronous versions of each top level function. Unfortunately, some of that seems to be inevitable since all functions end up calling
aact(either directly or through an asyncio.run call). We can remove this code duplication, but that would require parameterizing some these functions and introducing room for user errors. I thought it best to leave the duplication for now and in the future we can potentially break out some of the logic to separate functions (like creating an instruction from the component parts, etc...).Added a warning when calling async functions with a non-Simple context.
Added a session.clone() method to make interacting with contexts easier on a session level. (See feedback below)
Key concerns:
aact. This was to maintain consistency with sync functions and sampling strategies.a. Sync functions-
actrequires that we await the mot.avalue() call internally since the mot is being called from a non-async function that cannot awaitb. Sampling strategies- sampling strategies (along with generative slots) require the mot to be fully computed before validating (or for generative slots, applying the pydantic validation). This means that mots returned by
aactwhen using a sampling strategy must be already be computed. If we allowed users toawait mot.avalue()this would cause different behavior when sampling strategies are provided regardless of if the SamplingResults or the mot is returned.m.ctx = ctxonly after we've already awaited the mot's value. In many ways this is good, it means that we will never linearize a context with an uncomputed mot in it. However, it also means there's slightly wonky behavior if using non-Simple contexts with async functions: