-
Notifications
You must be signed in to change notification settings - Fork 199
#288: Support providing custom LLM for fact retrieval in the history #289
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
Qodana for JVM377 new problems were found
@@ Code coverage @@
+ 64% total lines covered
7573 lines analyzed, 4917 lines covered
# Calculated according to the filters of your coverage tool ☁️ View the detailed Qodana report Contact Qodana teamContact us at [email protected]
|
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 in general, just one concern about the way messages cleanup is implemented
...nts-features-memory/src/commonMain/kotlin/ai/koog/agents/memory/feature/nodes/MemoryNodes.kt
Outdated
Show resolved
Hide resolved
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.
Besides comment from @tiginamaria , the change LGTM!
...nts-features-memory/src/commonMain/kotlin/ai/koog/agents/memory/feature/nodes/MemoryNodes.kt
Show resolved
Hide resolved
...nts-features-memory/src/commonMain/kotlin/ai/koog/agents/memory/feature/nodes/MemoryNodes.kt
Show resolved
Hide resolved
agents/agents-core/src/commonMain/kotlin/ai/koog/agents/core/dsl/extension/AIAgentLLMActions.kt
Show resolved
Hide resolved
The PR is not linked to the issue that it fixes (only a commit is mentioned there), so here is the link: #288 |
84ebea6
to
a045616
Compare
Pls, update the description :) |
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.
Thank you, @Ololoshechkin
Just one suggestion
*/ | ||
public suspend fun saveFactsFromHistory( | ||
concept: Concept, | ||
subject: MemorySubject, | ||
scope: MemoryScope, | ||
retrievalModel: LLModel? = null |
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.
Can we make retrieval model required?
retrievalModel: LLModel? = null | |
retrievalModel: LLModel = model |
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.
No, it won't work, unfortunately, because model
is only available inside llm.writeSession
as it's a subject to change throughout the graph execution.
Fixes #288
Allows to select an optional custom model for fact retrieval from the history in memory nodes and history compression nodes
Type of the change
Checklist for all pull requests
develop
as the base branchAdditional steps for pull requests adding a new feature