[api][runtime][python] Add memory events for agent memory observability#887
[api][runtime][python] Add memory events for agent memory observability#887rosemarYuan wants to merge 4 commits into
Conversation
5b578ba to
6eff697
Compare
…-safe LTM buffer, observation context)
…s and generate-event config
6eff697 to
37fe38f
Compare
zxs1633079383
left a comment
There was a problem hiding this comment.
I took a light design pass, mainly around whether the new memory events pollute user payloads or collide with the event-log lineage direction.
The current shape looks good to me:
- Memory observations are emitted as separate typed events (
_short_term_write_event,_long_term_search_event, etc.) rather than mutating the triggering user event or putting framework fields into user-owned event attributes. - The memory event payload is scoped under
attributes: { key, value }, so the observation payload is explicit and reconstructable without changing the user event schema. - The action-finish flush boundary is a good place to fold per-action memory records into net-effect events, and the suppression for memory-triggered actions avoids recursive observation noise.
AgentRunBeginEventis kept as a lifecycle/reconstruction anchor rather than overloading memory events themselves.
One thing I would keep intentionally separate from this PR: lineage/run identity fields from #841. This PR should probably continue to avoid adding runId, sourceEventId, emittingAction, etc. directly into the memory event value payload. If/when the event-log lineage schema lands, those fields should sit in event context/envelope metadata, not inside the user-facing memory observation value map. The docs already point in that direction by describing key/value as the memory event attributes, so this is more of a boundary note than a blocker.
|
Thanks for the design review @zxs1633079383 — agreed, and that is exactly the boundary I want to keep. For this PR, the memory event payload under On the lineage direction, we are aligned. If/when the event-log lineage schema lands, those fields should be carried as event-log metadata/envelope-level information and apply uniformly to logged events, memory events included. The exact carrier shape, for example whether it is a separate Same principle you described: memory events expose memory observation data; lineage stays outside the user-facing event payload. |
weiqingy
left a comment
There was a problem hiding this comment.
Thanks for taking this on — the ordering in flushMemoryObservation is a nice call: draining the LTM buffer unconditionally before the suppressed/disabled early returns (RunnerContextImpl.java:226-243) is what stops a suppressed action from leaking records into the next same-key action. A few questions inline, plus one design-level one up top.
-
agent-run.begin-eventdefaulting totrue(MemoryEventOptions.java:58) carries two coupled costs that might be worth weighing together, since they share that one root. First, every input record now pays a fullMapState.entries()scan of the key's short-term memory inmaybeEmitAgentRunBeginEvent(ActionExecutionOperator.java:299-317) plus anAgentRunBeginEventallocation carrying the whole STM snapshot — even when the EventLog is disabled and nothing subscribes to consume it. On a RocksDB backend that's a state-backend iterator per record, not a cheap in-memory read. Second, because that scan reads every entry on every input and STM state is TTL-configured for read-refresh, the PR flips the repo-wide default ofshort-term-memory.state-ttl.update-typefromON_READ_AND_WRITEtoON_CREATE_AND_WRITE(AgentExecutionOptions.java:63-67) so expiry can still fire. That flip is global and unconditional — disablingagent-run.begin-eventdoesn't restore the old read-refresh semantics, so a user who enables STM TTL and never touches memory events still sees changed expiry behavior.Was defaulting the begin-event to
false— or gating emission on the EventLog being configured — considered? Either would drop the per-record scan for everyone not reconstructing STM from the log, and would make the global TTL default flip unnecessary, while still letting the feature be turned on when someone wants log-based reconstruction. I can also see the counter-argument: if the snapshot only starts when you enable it, there's no reconstruction anchor for runs that happened before you flipped it on, which is a real reason to keep it on by default. So this is genuinely a question about which default serves the common case better, not a claim that on-by-default is wrong — curious how you weighed the two. -
docs/content/docs/operations/configuration.md:128introduces its table as "the list of all built-in core configuration options", but the family this PR adds isn't in it — the 8memory.generate-event[.*]keys andagent-run.begin-eventare absent (grepping the page forbegin-event/agent-runreturns nothing). They're documented in the newmemory_events.md, so this is a completeness gap on the page that advertises itself as exhaustive, not a docs hole. Of the omitted keys,agent-run.begin-eventis the one a reader is most likely to come to this page looking for — its default is exactly what makes the TTL default flip in item (1) necessary. Adding the 9 keys to the table (or softening the "all" claim) would close it.
| } | ||
| Map<String, Object> flat = new LinkedHashMap<>(); | ||
| for (MemoryUpdate update : records) { | ||
| flat.put(update.getPath(), update.getValue()); |
There was a problem hiding this comment.
This puts the raw memory value object straight into the event's value map (update.getValue(), no normalization). Those events get drained into pendingEvents, persisted into ActionState.outputEvents, and re-emitted on replay via the completed-action path. The Kryo {serde,version,payload} envelope that preserves an untyped memory value's concrete type across durable recovery is bound class-scoped to MemoryUpdate.value (addMixIn(MemoryUpdate.class, ...) in ActionStateSerde) — it doesn't cover Event.attributes, which serializes through stock Jackson. So a byte[] short-term value (a supported type — ActionStateSerdeTest.testByteArrayMemoryValuePreserved) comes back as a base64 String after recovery, and a POJO as a LinkedHashMap. An action subscribing to _short_term_write_event would then see byte[] on the live run and String on the recovered run.
It only bites under a narrow combination — durable execution enabled, a non-JSON-native STM value, and an action that actually depends on the concrete type — so I don't think it blocks merge. And it may well be intentional: if memory events are audit-only, a base64 string is arguably fine. Is that the intent, or should the event path reuse the same envelope? Either way, testMemoryEventsSurviveActionStateRoundTrip currently uses only "gold" and 1 (both JSON-native, so type is trivially preserved) — a byte[] assertion there would pin whichever answer you land on.
| ) | ||
|
|
||
|
|
||
| class MemoryEventOptions: |
There was a problem hiding this comment.
There's an enforced parity guard that reflects a Java options class against its Python mirror and asserts the field names, keys, types, and defaults all match — check_java_python_config_options_parity.py, run in CI via test_java_config_in_python.sh. Its main() registers AgentConfigOptions and AgentExecutionOptions, but this new MemoryEventOptions family isn't wired in on either side. test_memory_event.py:104 looks like it fills that gap, but it checks the Python constants against hardcoded literals — it can't see Java, so by construction it can't catch Java↔Python drift. Given the 9 new keys and their tri-state null defaults are exactly the shape that's easiest to break asymmetrically, would registering the class in that checker's main() be worth it? normalize_java_default already maps a Java null default to Python None, so it should compare cleanly with no checker changes.
| return; | ||
| } | ||
| // ALWAYS drain LTM records for this key (mailbox thread) — discarded below if | ||
| // suppressed/disabled; skipping the drain would leak them into the next action. |
There was a problem hiding this comment.
The "ALWAYS drain" invariant here holds for suppressed/disabled actions — they finish normally and hit this drain before the early returns. But flushMemoryObservation only runs from drainEvents(_, true), which the action tasks call only on normal finish (JavaActionTask:85-86, and the Python task). When an action throws after doing LTM ops, invoke() rethrows, drainEvents(_, true) is never reached, and the Python-side buffer for this key isn't drained — so the guarantee is actually narrower than "a suppressed or disabled action still drains the buffer" reads. In practice it looks masked: a thrown action fails the mailbox task → operator restart → the interpreter's buffer is recreated fresh. I traced that path and restart appears to be the only exit, but I couldn't prove it exhaustively — is operator restart guaranteed to be the only way out here? A small test driving an action that throws and asserting no memory events leak into the next same-key action would settle both the invariant and my uncertainty.
| MemoryEventBuilder.parseDrainedRecords( | ||
| ltm.drainObservationRecordsJson(observationKeyHash)); | ||
| } catch (Exception e) { | ||
| LOG.debug("LTM observation drain failed; skipping", e); |
There was a problem hiding this comment.
If the Pemja drain_ltm_observation_records call throws, this swallows it at DEBUG and moves on. Two things go quiet: this action's LTM observation is lost, and if the Python side threw before clearing its buffer, those records survive and get attributed to the next same-key action's flush — the exact cross-action leak the drain exists to prevent. A persistent drain failure would misattribute LTM records with no visible signal. Would a throttled WARN fit better here, so the condition is at least observable?
| } else if ("DELETE".equals(op)) { | ||
| updates.put(set + "." + id, null); | ||
| } else if ("DELETE_SET".equals(op)) { | ||
| updates.put(set, null); |
There was a problem hiding this comment.
DELETE_SET adds set -> null but doesn't purge the earlier set + "." + id entries folded in above, so ADD s.m1=v then DELETE_SET s within one action yields {"s.m1": v, "s": null}. A log consumer reconstructing state from this event would see s.m1=v as a live member even though the same event's s -> null says the set — and that member with it — was deleted. It's observability-only, but the emitted value misreports the member. Purging the set + "." prefix entries when folding in a DELETE_SET would keep the reconstruction accurate.
Linked issue: #886
Purpose of change
Follow-up to #876, #886 .This change makes agent memory activity observable through the existing event infrastructure.
Emission model.
valueis a dot-key flat map describing the operation's net effect. Within one action, the last read/write for the same path wins.Recovery semantics. Generation remains consistent with the framework's existing delivery and durability guarantees:
durable_executeis not recorded again when its result is replayed from state. Generated events describe operations that actually executed in the current attempt.Tests
Coverage targets the design-critical paths:
MemoryEventWireFormatTestin Java andtest_memory_event_wire_format.pyin Python pin the same attribute JSON, preventing SDK drift.TestMemoryObservationFlushcovers emit-once-per-finished-action behavior, no emission on unfinished/failed drains, config gating, suppression awareness, and drain-then-discard semantics. A suppressed or disabled action still drains the long-term buffer, so records cannot leak into the next same-key action.TestMemoryEventSettingscovers all three resolution levels, including the case where the master switch is off but an operation-specific sub-key is explicitly on.TestMemoryEventBuildercovers empty paths, single paths, nested paths, last-wins behavior, and long-term add/delete/get/search value shapes.ActionStateSerdeTestverifies that memory events round-trip through the@classmixin, so they can be persisted and replayed with the action's output events.AgentRunBeginEventEmissionTest,AgentRunBeginEventTest, andtest_run_eventcover snapshot content, per-key isolation, and the guarantee that the snapshot is taken before the run's own writes.test_mem0_op_recordingandtest_mem0_recording_hookcover thread-safe buffering and drain-by-key behavior, including re-buffering of non-matching keys.ShortTermMemoryTTLIntegrationTestpins the newON_CREATE_AND_WRITEdefault.memory_event_logging_test.pyexercises the full emit → Event Log path.API
Java:
org.apache.flink.agents.api.event.MemoryEventorg.apache.flink.agents.api.event.ShortTermWriteEventorg.apache.flink.agents.api.event.ShortTermReadEventorg.apache.flink.agents.api.event.SensoryWriteEventorg.apache.flink.agents.api.event.SensoryReadEventorg.apache.flink.agents.api.event.LongTermUpdateEventorg.apache.flink.agents.api.event.LongTermGetEventorg.apache.flink.agents.api.event.LongTermSearchEventorg.apache.flink.agents.api.event.AgentRunBeginEventEventType.*constantsorg.apache.flink.agents.api.configuration.MemoryEventOptionsPython:
flink_agents.api.events.memory_eventflink_agents.api.events.run_eventflink_agents.api.core_options.MemoryEventOptionsThe new memory-event surface — event classes,
EventType.*constants, andMemoryEventOptions— does not depend on any long-term-memory backend.The observation recording seam is a runtime-side default-method extension point:
InternalBaseLongTermMemory.drainObservationRecordsJson, whose default implementation returns[]. A future pure-Java backend can therefore implement its own recording without another API change.Configuration. Memory event generation is controlled per operation kind. Configuration is resolved in the following order:
memory.generate-eventmaster switch.The built-in defaults are:
The master switch and operation-specific switches intentionally use
nulldefaults, so the built-in defaults remain reachable.agent-run.begin-eventhas its own switch, defaults to on, and is independent of thememory.generate-eventmaster switch.Notable behavior change.
short-term-memory.state-ttl.update-typechanges fromON_READ_AND_WRITEtoON_CREATE_AND_WRITE.ON_READ_AND_WRITE, that scan would refresh every entry's TTL and prevent expiry. Users who require read-refresh TTL semantics can set the option back explicitly, or disableagent-run.begin-event. See the documentation's Semantics & Caveats section for details.Documentation
doc-neededdoc-not-neededdoc-includedDocumentation is included in `docs/content/docs/development/memory/memory_events.mdThis change also adds notes to the sensory/short-term memory page and the configuration reference.