weiqingy commented on code in PR #1002:
URL: https://github.com/apache/flink-agents/pull/1002#discussion_r3888534945
##########
python/flink_agents/runtime/memory/mem0/mem0_long_term_memory.py:
##########
@@ -123,6 +123,23 @@ def validate_config(cls, v: Any, values: Any) -> Any:
return _FlinkAgentsLlmConfig, _FlinkAgentsEmbedderConfig
+def _bound_partition_key(memory_set: MemorySet) -> str:
+ """Return the partition key the set is scoped to.
+
+ Mem0 ignores a falsy ``agent_id`` rather than matching on it, so an
unbound set
+ would widen every operation to all keys sharing the job id and set name,
which
+ for a delete means deleting another key's items. Refuse the operation
instead.
+ """
+ if memory_set.partition_key is None:
Review Comment:
PR description updated.
On refusing rather than encoding: to keep already-stored memories readable
the mapping has to leave non-empty keys alone, which leaves no free non-empty
value for `""` to take. A reserved sentinel just moves which key breaks.
Collision-free would mean rewriting every key's `agent_id` and orphaning what
is already stored, so refusing looked like the better trade. Does that match
how you would weigh it?
Both `None` and `""` are refused now in both languages, with different
messages. The unbound default was `""` on both sides, so an unbound Java set
already looked bound by the time it reached Python.
Two things worth knowing. On `add`, mem0 drops `agent_id` from the stored
metadata, so items written under an empty key stay unrecoverable even after the
key is fixed. And an existing test was asserting that `agent_id=""` gets
forwarded, so this was pinned as intended behaviour.
##########
python/flink_agents/runtime/memory/mem0/mem0_long_term_memory.py:
##########
@@ -429,18 +446,33 @@ def drain_ltm_observation_records(self, key: str,
observation_id: str) -> str:
def get_memory_set(self, name: str) -> MemorySet:
"""Get the memory set by name.
+ The current partition key and observation context are copied onto the
set
+ so that operations submitted to a worker thread stay scoped to the
action
+ that obtained it. Must be called on the mailbox thread.
Review Comment:
Good point, done in both languages.
Both management methods now run the same mailbox check `getResource` uses.
The four set-scoped operations stay unchecked on purpose, since being safe on a
worker thread is what binding the context to the set buys.
Java needed its own check rather than relying on the bridge, since
`getMemorySet` there never enters Python. On the Python side I passed the
checker through the constructor rather than adding one to the `RunnerContext`
ABC, since Java's equivalent sits on the runtime class rather than the API
interface. Curious whether you would have put it on the interface instead.
`getLongTermMemory` is left alone, since capturing the handle on the mailbox
thread and using it inside the callable is normal.
This settles the `delete_memory_set` question without touching its
signature. One caveat: on JDK 11 the durable callable runs inline, so the same
code will not raise there yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
