fix: normalize NeptuneDriver params and AOSS bulk actions#1539
Conversation
17e1b81 to
29af3dc
Compare
|
I have read the CLA Document and I hereby sign the CLA behalf on myself, e-mail: [email protected] or I have read the CLA Document and I hereby sign the CLA behalf of my company, e-mail: [email protected] Signature is valid for 6 months. This bot will be retriggered when the Contributor License Agreement comment has been provided. Posted by the CLA Assistant Lite bot. |
PR Triage AssessmentPriority: MEDIUM | Category: bug-fix | Action: merge-ready SummaryFixes three concrete bugs in the existing Quality Scores
Signals
Maintainer NoteSolid, surgically-scoped bug fix on a non-primary backend (Neptune), with unit tests covering each behavioral change. No live AWS integration test (acknowledged by the author and reasonable given mocked clients). Worth a quick review of the Note to AuthorThanks for the well-scoped bug fix and accompanying unit tests — this looks ready for maintainer review. One process item: please sign the Contributor License Agreement (see the bot comment above) so the PR can be merged. Raw triage data (JSON){
"pr": 1539,
"category": "bug-fix",
"priority": "MEDIUM",
"recommended_action": "merge-ready",
"quality_scores": { "tests": 2, "documentation": 3, "code_style": 3, "pr_scope": 2, "total": 10 },
"signals": { "follows_patterns": "yes", "focused_scope": true, "has_rfc_if_needed": "n/a" },
"slop_signals": [],
"duplicate_of": null,
"impact": { "references_issue": "#1529", "touches_core": false, "affects_primary_backend": false, "backend": "neptune (non-primary)" },
"loc": { "additions": 142, "deletions": 5, "changed_files": 2 },
"notes": "CLA not yet signed; no live Neptune/AOSS integration test (mocked unit tests only, acknowledged by author)."
} |
|
I have read the CLA Document and I hereby sign the CLA behalf on myself, e-mail: [email protected] |
|
Thanks @cnYui for picking this up, and thanks for the focused unit tests. I opened #1529, and this PR covers the first failures I reported: the I also ran a local compatibility shim against a live Amazon Neptune Database + Amazon OpenSearch Serverless setup. With that shim, On top of what's already in this PR, I hit two more live-path points that don't seem covered yet:
These are additive and out of scope for this PR's clean fix. I'm happy to either add focused review suggestions here if you'd prefer to keep it in one PR, or open a small follow-up PR referencing #1529 and this one and rebase after this merges. Whichever you and the maintainers prefer. |
|
@bonajoy The live Neptune/AOSS validation is useful confirmation, especially because this PR only has mocked unit coverage. I agree the two additional live-path points are worth addressing, but I would prefer to keep this PR focused on the first failures from #1529 so it stays easy to review and merge: the The My preference is to open a follow-up PR after this one lands, referencing #1529 and #1539, for:
If maintainers prefer to handle those as review suggestions on this PR, I can take that route, but I would avoid expanding this PR unless that is the preferred review path. |
|
Thanks @cnYui — agreed on keeping this PR focused on the first failures so it stays easy to review and merge. Splitting the two live-path points into a follow-up after this lands makes sense to me. Since I already have a live Amazon Neptune + AOSS environment available from validating #1529 and have these tweaks working locally, I'm happy to take the follow-up PR if that works for you and the maintainers. Otherwise, I'm also happy to help review or provide live validation for it. I'd keep the follow-up small and focused on:
Either way, I'll wait for this PR to land so the follow-up can be rebased cleanly on Thanks again for picking up the first fix set. |
jhurliman
left a comment
There was a problem hiding this comment.
Independently arrived at the same two fixes while debugging #1529, and can confirm this PR resolves both root causes end-to-end.
Validated on current main against Amazon Neptune Serverless (neptune-db://) + OpenSearch Serverless (Bedrock Titan v2 embeddings + Haiku extraction via an OpenAI-compatible gateway):
self._database = database(default'') fixes theAttributeErrorinadd_episodewhen an explicitgroup_idis passed (graphiti.pyif group_id != self.driver._database). The''default lines up withget_default_group_id(GraphProvider.NEPTUNE)returning''.- Popping/normalizing
paramsinexecute_queryfixes theMalformedQueryException: MissingParameterthat otherwise hits everyadd_episodeviasearch_utils.node_similarity_search(params=filter_params).
With both applied, add_episode, hybrid search, current-state recall, and explicit edge supersession all work on Neptune Serverless + AOSS. LGTM. (#1568 also addresses the params half, with the nice extra touch of filtering to only $-referenced params.)
Summary
Addresses part of #1529.
NeptuneDriver._databaseand implementclone(database=...)so group-scopedGraphiti.add_episodepaths do not hit the base driver no-op cloneNeptuneDriver.execute_query()parameters by flattening Neo4j-styleparams={...}and droppingrouting_before calling Neptune_idvalues in AOSS bulk actions while keeping logicaluuidin the indexed source fieldsTests
.\.venv\Scripts\python.exe -m pytest tests/driver/test_neptune_driver.py -q.\.venv\Scripts\python.exe -m pytest tests/driver -q.\.venv\Scripts\python.exe -m ruff format --check graphiti_core/driver/neptune_driver.py tests/driver/test_neptune_driver.py.\.venv\Scripts\python.exe -m ruff check graphiti_core/driver/neptune_driver.py tests/driver/test_neptune_driver.py.\.venv\Scripts\python.exe -m pyright --pythonpath .\.venv\Scripts\python.exe graphiti_core/driver/neptune_driver.pyNotes
This does not add a live Neptune/AOSS integration test. The new tests mock the driver clients so the contract is covered without requiring AWS resources.