Repository navigation
Conversation
It is the reproduction according to the source repo https://github.com/WujiangXu/A-mem-sys . The retreival of neighbor note (through note links) is self implemented as not well described in the paper and the source repo. For the interpretation of the policy, please refer to the comments.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (10)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThis pull request adds the A-Mem memory policy, a Neo4j storage profile, public package exports, and a LOCOMO end-to-end demonstration. The demonstration checks retrieval and restored state, and writes run metrics and reproducibility artifacts. ChangesA-Mem memory policy
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Demo as e2e_demo.py main()
participant Memory as AMem
participant Storage as Neo4j storage
Demo->>Memory: Add selected LOCOMO turns
Memory->>Storage: Store note state and links
Demo->>Memory: Run retrieval query
Memory-->>Demo: Return retrieval results
Demo->>Storage: Checkpoint runtime state
Demo->>Storage: Restore state through a fresh connection
Merge Risk: ⚪ Minimal · up to No demonstrated A-Mem storage issue remains that would block merging after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 68.54% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 89 functions across 9 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SleepyLGod
left a comment
SleepyLGod
left a comment
There was a problem hiding this comment.
Review
From Von
This is a good starting point, but it is not yet a Native-aligned A-MEM implementation. Please address the following before merging later.
1. Choose One Reference Implementation
Both repositories are official, but serve different purposes:
[WujiangXu/A-mem](https://github.com/WujiangXu/A-mem), previouslyAgenticMemory: the paper-reproduction repository.[WujiangXu/A-mem-sys](https://github.com/WujiangXu/A-mem-sys): the application-oriented implementation.
For our research comparison, please use a pinned commit of A-mem as the baseline. Keep prompts, constants, storage configuration, and retrieval behavior consistent with that version.
2. Issues In This PR
- Array parsing: The final multi-output
sem_aggexpects an array-valuedtags, but the production parser only accepts scalar fields. The fake adapter bypasses this limitation. Use the existing single-JSON-column →unnestpattern and test the real parsing path. - Update semantics: The new consolidation prompt preserves historical context/tags. Native replaces them with the evolution output. This changes the memory semantics; aggregation should represent the intended final state, not introduce another summarization policy.
- Evolution inputs: Later decisions read frozen initial analyses rather than evolved notes. Please verify the proposed view and generated maintenance query against the selected Native implementation.
- Candidate selection: All earlier notes enter evolution, whereas Native uses vector top-5 neighbors. This needs an explicit, Native-aligned physical candidate configuration.
- Embedding assembly:
sem_mapshould not concatenate strings. It adds unnecessary LLM cost and cannot guarantee exact formatting. - Retrieval: Native already implements outgoing-link expansion in
find_related_memories_raw(). Follow that implementation rather than guessing. Preserve the returned metadata and verify expansion limits. Neo4j/BGE is also a different physical configuration from the research baseline.
3. What Our Repository Still Needs
We lack small deterministic expressions for string concatenation and joining array elements into text. assign() exists, but these expressions do not.
Native-aligned vector-index integration and maintenance candidate lookup also need additional work. Please flag these gaps rather than replace them with LLM operations or undocumented behavior.
4. Recommended Next Step
First pin the research repository and write a short Native-to-policy mapping. Then fix parsing, update semantics, and retrieval; add only the missing general-purpose capabilities. Validate both the production parser and generated maintenance behavior. Passing fake-adapter tests alone does not establish Native parity.
Feel free to use this for your local testing: https://build.nvidia.com/models
SleepyLGod
changed the title
SleepyLGod
changed the title
2 participants
It is the reproduction according to the source repo https://github.com/WujiangXu/A-mem-sys . The retreival of neighbor note (through note links) is self implemented as not well described in the paper and the source repo. For the interpretation of the policy, please refer to the comments in policy.py.
Summary by CodeRabbit