-
Notifications
You must be signed in to change notification settings - Fork 76
[api][runtime] Introduce long-term memory in python #332
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
rename memory item
e85caea to
7c14985
Compare
| from flink_agents.api.prompts.prompt import Prompt | ||
|
|
||
|
|
||
| class ReduceStrategy(Enum): |
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.
Not sure about the name Reduce. I think Compact might be better.
| SUMMARIZE = "summarize" | ||
|
|
||
|
|
||
| class ReduceSetup(BaseModel): |
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'd suggest to name this CompactionStrategy, and make it an abstract class that we can provide different implementations, so we can have strict limit on which arguments should be specified for each strategy. We can call the current ReduceStrategy CompactionStrategyType.
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 think CompactionStrategy.trim(n) might be more straightforward for users, compared to ReduceSetup.trim_setup(n).
| id: str | ||
| value: Any | ||
| compacted: bool = False | ||
| created_time: DatetimeRange |
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.
| created_time: DatetimeRange | |
| created_time: DatetimeRange | datetime |
| size: int = 0 | ||
| capacity: int | ||
| reduce_setup: ReduceSetup | ||
| item_ids: List[str] = Field(default_factory=list) |
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.
Why do we need to store ids of all the items?
| capacity: int | ||
| reduce_setup: ReduceSetup | ||
| item_ids: List[str] = Field(default_factory=list) | ||
| reduced: bool = False |
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.
And why do we need to know whether a memory set has been reduced/compacted?
| ) | ||
|
|
||
| # Connection configuration | ||
| persist_directory: str | None = Field( |
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.
what is this directory for?
| if memory_set.size >= memory_set.capacity: | ||
| # trigger reduce operation to manage memory set size. | ||
| self._reduce(memory_set) |
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 can be extremely slow. We should proactively do the compaction.
| self.client.delete_collection(name=name) | ||
|
|
||
| @override | ||
| def add(self, memory_set: MemorySet, memory_item: str | ChatMessage) -> 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 had a feeling that adding items to long-term memory can take time, for embedding. We probably should also provide async apis.
| return self.slice(memory_set=memory_set, offset=offset, n=n) | ||
|
|
||
| @override | ||
| def search( |
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.
Same here.
| def _trim(self, memory_set: MemorySet) -> None: | ||
| reduce_setup: ReduceSetup = memory_set.reduce_setup | ||
| n = reduce_setup.arguments.get("n") | ||
| self.delete(memory_set=memory_set, offset=0, n=n) | ||
|
|
||
| def _summarize(self, memory_set: MemorySet) -> 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.
Are these methods specialized for this class?
|
Hi, @alnzng. There's a design issue related to the vector store that I'd appreciate your help reviewing. As describe in the design doc #339, long-term memory of flink-agents is also based on vector store. Currently, I provide an implementation based on chroma. In this implementation, I directly use chroma client rather than flink-agents @xintongsong believes that we can directly build upon the Flink-Agents I think this make sense, but it requires add some interfaces to These interface may not be achievable for each vector store, I will conduct research and refinement afterward. |
Linked issue: #331
Purpose of change
Introduce the long-term memory interface in python, and provide an implementation based on chroma.
This is the first pr of three to introduce long-term memory in python:
Tests
Unit test
API
Yes, add long-term memory related api.
Documentation
doc-needed