-
Notifications
You must be signed in to change notification settings - Fork 593
feat: Add graph metrics getter in graph db interface and adapters [COG-1082] #483
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…-tokens-to-metric-table
…add-num-tokens-to-metric-table
WalkthroughThe changes introduce a new asynchronous method Changes
Sequence Diagram(s)sequenceDiagram
participant S as store_descriptive_metrics
participant GE as Graph Engine
participant DB as Database
S->>GE: await get_graph_metrics()
GE-->>S: Return metrics dictionary (values: -1)
S->>DB: Create & commit GraphMetrics object
Suggested labels
Suggested reviewers
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cognee/infrastructure/databases/graph/graph_db_interface.py (1)
57-60
: Add return type hint for better type safety.The new abstract method should specify its return type for better type safety and documentation.
@abstractmethod - async def get_graph_metrics(self): + async def get_graph_metrics(self) -> dict[str, float]: raise NotImplementedError
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cognee/infrastructure/databases/graph/graph_db_interface.py
(1 hunks)cognee/infrastructure/databases/graph/neo4j_driver/adapter.py
(1 hunks)cognee/infrastructure/databases/graph/networkx/adapter.py
(1 hunks)cognee/tasks/storage/descriptive_metrics.py
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: run_simple_example_test / test
- GitHub Check: run_multimedia_example_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_simple_example_test / test
- GitHub Check: run_dynamic_steps_example_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: run_notebook_test / test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: windows-latest
- GitHub Check: docker-compose-test
- GitHub Check: profiler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cognee/modules/data/methods/store_descriptive_metrics.py (1)
28-52
: Consider adding metrics validation and logging.The function would benefit from additional validation and logging to track metrics over time.
Consider:
- Adding range validation for metrics (e.g., non-negative values)
- Adding logging to track metrics changes
- Adding comparison with previous metrics for monitoring trends
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cognee/api/v1/cognify/cognify_v2.py
(1 hunks)cognee/modules/data/methods/store_descriptive_metrics.py
(2 hunks)cognee/modules/data/models/__init__.py
(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- cognee/api/v1/cognify/cognify_v2.py
- cognee/modules/data/models/init.py
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: run_notebook_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
- GitHub Check: test
- GitHub Check: test
- GitHub Check: run_simple_example_test / test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: run_notebook_test / test
- GitHub Check: test
- GitHub Check: windows-latest
- GitHub Check: test
- GitHub Check: test
- GitHub Check: docker-compose-test
- GitHub Check: profiler
🔇 Additional comments (1)
cognee/modules/data/methods/store_descriptive_metrics.py (1)
10-10
: LGTM!The import statement change looks appropriate for the module organization.
735c2a8
to
05138fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cognee/modules/data/methods/store_descriptive_metrics.py (2)
33-50
: Consider using a transaction context manager.The database operations should be wrapped in a transaction to ensure atomicity. If any operation fails, all changes should be rolled back.
Apply this diff to add transaction handling:
- async with db_engine.get_async_session() as session: + async with db_engine.get_async_session() as session: + async with session.begin(): metrics = GraphMetrics( # ... existing metrics initialization ... ) session.add(metrics) - await session.commit()
28-52
: Implementation provides a good foundation for graph metrics collection.The implementation successfully demonstrates the interface by collecting standard graph metrics (nodes, edges, clustering, etc.). This provides a solid foundation for future enhancements.
Consider these future enhancements:
- Add caching for expensive metrics calculations
- Implement batch processing for large graphs
- Add metric calculation timeouts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cognee/api/v1/cognify/cognify_v2.py
(1 hunks)cognee/modules/data/methods/__init__.py
(1 hunks)cognee/modules/data/methods/store_descriptive_metrics.py
(2 hunks)cognee/modules/data/models/__init__.py
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cognee/modules/data/models/init.py
- cognee/api/v1/cognify/cognify_v2.py
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: run_notebook_test / test
- GitHub Check: windows-latest
- GitHub Check: run_simple_example_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_notebook_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: test
- GitHub Check: run_dynamic_steps_example_test / test
- GitHub Check: test
- GitHub Check: docker-compose-test
- GitHub Check: test
- GitHub Check: profiler
🔇 Additional comments (2)
cognee/modules/data/methods/store_descriptive_metrics.py (2)
31-31
: Add error handling for graph metrics retrieval.The direct call to
get_graph_metrics()
should be wrapped in a try-catch block to handle potential errors gracefully.
34-47
: Add validation and type hints for graph metrics.The code assumes all required metrics exist in the dictionary without validation. Also, type hints would improve code clarity.
05138fa
to
268d778
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cognee/modules/data/methods/store_descriptive_metrics.py (2)
11-23
: Add error handling for token count retrieval.The function should handle edge cases where the token count might be null or when database errors occur.
Apply this diff to improve error handling:
async def fetch_token_count(db_engine) -> int: """ Fetches and sums token counts from the database. Returns: int: The total number of tokens across all documents. + + Raises: + RuntimeError: If token count retrieval fails """ async with db_engine.get_async_session() as session: - token_count_sum = await session.execute(select(func.sum(Data.token_count))) - token_count_sum = token_count_sum.scalar() + try: + token_count_sum = await session.execute(select(func.sum(Data.token_count))) + token_count_sum = token_count_sum.scalar() or 0 # Handle NULL case + except Exception as e: + raise RuntimeError(f"Failed to retrieve token count: {str(e)}") from e return token_count_sum
26-26
: Add docstring to explain function purpose and parameters.The function lacks documentation explaining its purpose, parameters, and return value.
Apply this diff to add documentation:
async def store_descriptive_metrics(data_points: list[DataPoint]): + """ + Store descriptive metrics about the graph structure and token usage. + + This function retrieves graph metrics from the graph engine and token count + from the relational database, then stores them as a new GraphMetrics entry. + + Args: + data_points: List of DataPoint objects being processed + + Returns: + list[DataPoint]: The input data_points list unchanged + + Note: + This function is part of the metrics collection pipeline and stores + various graph metrics like node count, edge density, clustering, etc. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cognee/api/v1/cognify/cognify_v2.py
(1 hunks)cognee/modules/data/methods/__init__.py
(1 hunks)cognee/modules/data/methods/store_descriptive_metrics.py
(1 hunks)cognee/modules/data/models/__init__.py
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cognee/modules/data/models/init.py
- cognee/modules/data/methods/init.py
- cognee/api/v1/cognify/cognify_v2.py
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: run_dynamic_steps_example_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_simple_example_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: run_notebook_test / test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: windows-latest
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: docker-compose-test
- GitHub Check: profiler
🔇 Additional comments (1)
cognee/modules/data/methods/store_descriptive_metrics.py (1)
29-29
: Add error handling for graph metrics retrieval.The direct call to
get_graph_metrics()
should be wrapped in a try-catch block to handle potential errors gracefully.
Dummy implementation of graph metrics to demonstrate how the interface will look like
Description
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
New Features
Refactor
GraphMetrics
database table and reorganized module imports to enhance internal consistency.Chores