-
Notifications
You must be signed in to change notification settings - Fork 75
feat: add stream support to mfuncs and session #395
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
base: main
Are you sure you want to change the base?
Conversation
|
The PR description has been updated. Please fill out the template for your PR to be reviewed. |
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/
|
|
Thank you. That looks great. We might still have to create a new issue on how to handle streaming with requirement-checking and sampling strategies. @jakelorocco and @nrfulton |
Agreed. While doing this we should think about how much we can stream sampling. |
jakelorocco
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.
Will follow up / answer the questions I indicated tomorrow morning. Thank you! This looks good.
mellea/stdlib/functional.py
Outdated
| await_result: bool = True, | ||
| ) -> tuple[ModelOutputThunk[S], Context] | SamplingResult: |
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.
Lets set this to False by default. I think that's likely a better longterm default and the expected behavior. Will confirm with the team tomorrow though. I know this is an api breaking change too, so we will just need to flag it in the release notes.
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.
Others agreed that we should set this to False by default.
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.
This change requires making sure all tests and examples working without failure. It may take some time but it will be done.
mellea/stdlib/functional.py
Outdated
| await_result: bool = True, | ||
| ) -> tuple[ModelOutputThunk[S], Context] | SamplingResult: |
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.
We should be able to add type hinting so that if await_result == True or strategy != None, we return a computed model output thunk.
mellea/stdlib/functional.py
Outdated
| # ._generate_log should never be None after generation. | ||
| assert result._generate_log is not None | ||
| result._generate_log.is_final_result = True | ||
| generate_logs.append(result._generate_log) |
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.
Let me think about this generate_log change. I have to review where they are being used / if they are still being used anywhere.
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.
I believe we can actually get rid of the generate_logs.append(result._generate_log) lines in this function.
mellea/core/base.py
Outdated
| class ComputedModelOutputThunk(ModelOutputThunk[S]): | ||
| """A `ComputedModelOutputThunk` is a `ModelOutputThunk` that is guaranteed to be computed. | ||
|
|
||
| This subclass provides a clear type distinction between thunks that may need awaiting | ||
| and those that are already computed. It should be returned from synchronous functions | ||
| and sampling strategies to indicate that no awaiting is needed. | ||
|
|
||
| Key differences from ModelOutputThunk: | ||
| - Always initialized with a value (cannot be None) | ||
| - _computed is always True | ||
| - Cannot be used for streaming (generation fields are not set) | ||
| - Provides type safety to indicate "already computed" | ||
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
| value: str, | ||
| meta: dict[str, Any] | None = None, | ||
| parsed_repr: S | None = None, | ||
| tool_calls: dict[str, ModelToolCall] | None = None, | ||
| ): |
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.
I feel like this is a bit clunky to create a computed model output thunk. I don't think we will ever have to create one from scratch. Is it possible to have the constructor just take the regular model output thunk as it's only arg, check that everything is computed, set computed to true, and then just defer all of it's getters to the base model output thunk? Or maybe theres some easier way to do this where we don't have to copy over all of the fields.
mellea/stdlib/functional.py
Outdated
|
|
||
| Returns: | ||
| A (ModelOutputThunk, Context) if `return_sampling_results` is `False`, else returns a `SamplingResult`. | ||
| Always returns ComputedModelOutputThunk since sync functions must await completion. |
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.
Where possible, with the sync functions, can we change the return type hinting to be the computed model output thunk?
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.
This should be removed.
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Misc PR
Type of PR
Description
Testing