Feature/cog 1369 quick fix dynamic collection handling in search#567
Conversation
With the new dynamic collection handling edge collection is also getting mapped to nodes which will produce error messages.
WalkthroughThe pull request updates error handling and import logic in the graph modules and exception classes. In the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Client
participant CG as map_vector_distances_to_graph_nodes
participant Lookup as Node Lookup
Caller->>CG: Invoke mapping with input vector IDs
CG->>Lookup: Search for node by ID
alt Node exists
Lookup-->>CG: Return node data
CG->>Caller: Process mapping with node data
else Node missing
Lookup-->>CG: No node found
CG->>Caller: Proceed silently (no error message)
end
sequenceDiagram
participant Client as Client
participant GQC as graph_query_completion
participant DP as DataPoint
Client->>GQC: Request graph query completion
GQC->>DP: Execute get_all_subclasses(DataPoint)
DP-->>GQC: Return list of subclasses
GQC->>Client: Process query and provide results
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…llection handling it is normal and not an error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
cognee/infrastructure/databases/exceptions/exceptions.py (1)
5-14: 💡 Verification agent🧩 Analysis chain
Consider maintaining consistency in error hierarchy.
The change from
CogneeApiErrortoExceptionas the base class creates an inconsistency in the error hierarchy, asEntityAlreadyExistsErrorstill inherits fromCogneeApiError. This might lead to inconsistent error handling across the codebase.Let's verify the impact of this change:
🏁 Script executed:
#!/bin/bash # Search for catch blocks handling these exceptions rg -A 3 "except (EntityNotFoundError|CogneeApiError)"Length of output: 432
Action Required: Refactor Error Hierarchy for Consistency
The recent change in
EntityNotFoundError—switching its inheritance fromCogneeApiErrorto the builtinException—creates an inconsistency with other error classes (e.g.,EntityAlreadyExistsErrorstill inherits fromCogneeApiError). Although the catch block incognee/infrastructure/databases/vector/pgvector/PGVectorAdapter.pyexplicitly handlesEntityNotFoundError, this split in the error hierarchy could lead to unpredictable handling when catching a generalCogneeApiErrorin other parts of the codebase.
- Impact: If parts of the application rely on a common base for error handling (i.e.,
CogneeApiError), they might not catchEntityNotFoundErroras expected.- Recommendation: Revisit the error hierarchy and either have all custom errors (including
EntityNotFoundError) extendCogneeApiErroror refactor the others for uniformity.
🧹 Nitpick comments (1)
cognee/infrastructure/databases/vector/pgvector/PGVectorAdapter.py (1)
218-220: Consider logging table not found errors.Silently returning an empty list when a collection is not found might hide potential issues. Consider adding debug logging to help track these occurrences.
except EntityNotFoundError: + logging.debug(f"Collection {collection_name} not found, returning empty list") # Ignore if collection does not exist return []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cognee/infrastructure/databases/exceptions/exceptions.py(1 hunks)cognee/infrastructure/databases/vector/pgvector/PGVectorAdapter.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (28)
- GitHub Check: run_multimedia_example_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_eval_framework_test / test
- GitHub Check: run_networkx_metrics_test / test
- GitHub Check: Test on macos-15
- GitHub Check: Test on macos-15
- GitHub Check: Test on macos-15
- GitHub Check: run_simple_example_test / test
- GitHub Check: run_dynamic_steps_example_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: Test on macos-13
- GitHub Check: Test on macos-13
- GitHub Check: Test on macos-13
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: windows-latest
- GitHub Check: test
- GitHub Check: Build Cognee Backend Docker App Image
- GitHub Check: docker-compose-test
cognee/infrastructure/databases/vector/pgvector/PGVectorAdapter.py
Outdated
Show resolved
Hide resolved
…namic collection handling it is normal and not an error" This reverts commit 048fb88.
…-handling-in-search
…n handling it is not an error anymore
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
cognee/infrastructure/databases/exceptions/exceptions.py (2)
5-5:⚠️ Potential issueInconsistency with PR objectives detected.
The AI summary indicates that
EntityNotFoundErrorshould inherit fromException, but it's still inheriting fromCogneeApiError. This inconsistency needs to be resolved.Apply this diff to align with the PR objectives:
-class EntityNotFoundError(CogneeApiError): +class EntityNotFoundError(Exception):
20-29: 💡 Verification agent🧩 Analysis chain
Verify consistency in exception handling approach.
Given the changes to
EntityNotFoundError, shouldEntityAlreadyExistsErrorfollow the same pattern? Please verify if this class should also:
- Inherit from
Exceptioninstead ofCogneeApiError- Update its error logging behavior
🏁 Script executed:
#!/bin/bash # Search for usage patterns of both exception classes echo "Searching for EntityNotFoundError usage..." rg "EntityNotFoundError" -A 5 echo "Searching for EntityAlreadyExistsError usage..." rg "EntityAlreadyExistsError" -A 5Length of output: 22687
Action Required: Standardize Exception Initialization
After reviewing the usage across the codebase, here are the findings:
- Both
EntityNotFoundErrorandEntityAlreadyExistsErrorconsistently inherit fromCogneeApiErroracross modules. There’s no indication that either should inherit directly fromException.- In the infrastructure module (
cognee/infrastructure/databases/exceptions/exceptions.py), the two exception classes use different initialization patterns:
EntityNotFoundErrorassigns instance attributes directly.EntityAlreadyExistsErrorcallssuper().__init__.- There’s no evidence from the usage that an updated error logging behavior is necessary.
Recommendation:
For consistency, consider aligning the__init__implementation of both exception classes. If the new pattern (direct attribute assignment) forEntityNotFoundErroris intentional, then updateEntityAlreadyExistsErrorto follow suit. Alternatively, revert the change inEntityNotFoundErrorto usesuper().__init__.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cognee/infrastructure/databases/exceptions/exceptions.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (28)
- GitHub Check: run_dynamic_steps_example_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_eval_framework_test / test
- GitHub Check: Test on macos-15
- GitHub Check: Test on macos-15
- GitHub Check: Test on macos-13
- GitHub Check: Test on macos-15
- GitHub Check: run_notebook_test / test
- GitHub Check: Test on macos-13
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: Test on macos-13
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: Test on ubuntu-22.04
- GitHub Check: run_networkx_metrics_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_simple_example_test / test
- GitHub Check: test
- GitHub Check: run_multimedia_example_test / test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: run_notebook_test / test
- GitHub Check: windows-latest
- GitHub Check: docker-compose-test
- GitHub Check: Build Cognee Backend Docker App Image
🔇 Additional comments (2)
cognee/infrastructure/databases/exceptions/exceptions.py (2)
17-17:Details
✅ Verification successful
Verify the impact of disabling error logging.
The TODO comment suggests that error logging is being disabled due to dynamic exception handling. Let's verify if this change aligns with the error handling requirements across the codebase.
🏁 Script executed:
#!/bin/bash # Search for error logging patterns and exception handling in the codebase echo "Searching for error logging patterns..." rg "log.*error|log.*exception" -A 5 echo "Searching for dynamic exception handling patterns..." rg "except.*as.*:|raise.*Exception|raise.*Error" -A 5Length of output: 69040
Impact of Disabling Internal Error Logging in Exceptions Module
After verifying the repository’s error logging practices, it’s clear that the dynamic exception handling strategy now logs errors at higher levels (e.g., in service and route handlers) rather than within the exception class itself. Disabling internal error logging in
cognee/infrastructure/databases/exceptions/exceptions.pyis consistent with this approach and avoids redundant log entries.
- The commented-out logger call in the exceptions module aligns with the established pattern found throughout the codebase.
- All critical errors continue to be logged externally when caught, ensuring no loss in error visibility.
Overall, this change should have a minimal impact on error tracking while streamlining error handling.
1-2:Details
✅ Verification successful
Review import requirements.
If both exception classes are changed to inherit from
Exception, theCogneeApiErrorimport would no longer be needed.
🏁 Script executed:
#!/bin/bash # Check if CogneeApiError is used elsewhere in this directory rg "CogneeApiError" ./cognee/infrastructure/databases/Length of output: 588
Ensure consistency in your exception inheritance strategy.
The search confirms that both exception classes in
cognee/infrastructure/databases/exceptions/exceptions.pystill inherit fromCogneeApiErrorand that it’s also used inEmbeddingException.py. Therefore, the import ofCogneeApiErrorremains necessary. If you plan on refactoring these exceptions to inherit directly from Python’s built-inException, please ensure that all related exception classes across the codebase are updated accordingly.
…-handling-in-search
…-handling-in-search
…-handling-in-search
|
Tests are failing because of the benchmark adapter unit tests from dev (google drive ban) otherwise this can be merged. |
…-handling-in-search
…-handling-in-search
borisarzentar
deleted the
feature/cog-1369-quick-fix-dynamic-collection-handling-in-search
branch
- Removed 5 legacy caching TODOs from cache_db_interface.py, test_fs_adapter_crud.py, and test_redis_adapter_crud.py referencing backward compatibility that was completed in PR topoteretes#2077. - Removed 1 outdated dynamic exception handling TODO attached to dead code in exceptions.py from PR topoteretes#567.
2 participants
Description
Fixes search dynamic collection mapping in graph completion search
DCO Affirmation
I affirm that all code in every commit of this pull request conforms to the terms of the Topoteretes Developer Certificate of Origin
Summary by CodeRabbit