Support new LLMContext pattern with AWSNovaSonicLLMService
#2750
+366
−473
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
…textAggregatorPair`
kompfner
force-pushed
the
pk/aws-nova-sonic-universal-llmcontext-1
branch
from
ec0baa6 to
4d9873b
Compare
kompfner
commented
Sep 29, 2025
| ) | ||
| return filtered_messages | ||
|
|
||
| def get_messages_for_persistent_storage( |
There was a problem hiding this comment.
This is new functionality on LLMContext. There are corresponding unit tests in this PR.
The reason for the system_instruction argument is to support usage with LLMs where you might pass the system instruction as a parameter to the LLMService rather than via the context.
Pending: I'll update some examples to highlight this functionality.
There was a problem hiding this comment.
Hmm...as I think about this more I'm not sure we need the complexity of the system_instruction argument...
- If you specified your system instruction in the context in the first place, it'll still be there when you read messages for persistent storage
- If you didn't specify your system instruction in the context and instead passed it in as an LLM service parameter, you probably don't want it to be in the context when you read messages for persistent storage
- ...and if you really really do need to inject it at the start of the context, it's quite easy to do yourself
There was a problem hiding this comment.
Which means...we probably don't need this method at all. We can just rely on get_messages().
The reason for its `system_instruction` argument was to support usage with LLMs where you might pass the system instruction as a parameter to the `LLMService` rather than specifying it in the context. But as I thought about it more I became unconvinced that the `system_instruction` argument was really beneficial: - If you specified your system instruction in your context in the first place, it'll still be there when you read messages for persistent storage - If you didn't specify your system instruction in the context and instead passed it in as an `LLMService` parameter, you most likely *don't* want it to be in the context when you read messages for persistent storage - ...and if you really really do need to inject it at the start of the context, it's quite easy to do anyway And if we remove the `system_instruction` argument from `get_messages_for_persistent_storage()`, then it's essentially just `get_messages()`.
kompfner
commented
Sep 29, 2025
| tools: Available tools/functions for the model to use. | ||
| send_transcription_frames: Whether to emit transcription frames. | ||
| .. deprecated:: 0.0.87 |
There was a problem hiding this comment.
Note to self: update this if this doesn't make the 0.0.87 release
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.