-
Notifications
You must be signed in to change notification settings - Fork 86
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
feat: Add ability to change graph database configuration through cognee #421
Conversation
WalkthroughThe pull request introduces a new static method Changes
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: 1
🧹 Nitpick comments (2)
cognee/api/v1/config/config.py (2)
135-135
: Add type hints for dictionary keys and values.The
config_dict
parameter lacks specific type hints for its contents. This could lead to type-related issues at runtime.Apply this diff to add specific type hints:
- def set_graph_db_config(config_dict: dict) -> None: + def set_graph_db_config(config_dict: dict[str, Any]) -> None:Don't forget to add the import:
from typing import Any
136-138
: Enhance docstring with parameter and raises information.The current docstring is basic and could be more descriptive about the expected input and potential exceptions.
Consider updating the docstring:
- """ - Updates the graph db config with values from config_dict. - """ + """Updates the graph database configuration with the provided values. + + Args: + config_dict: Dictionary containing configuration key-value pairs. + Keys must be valid attributes of the graph database config. + + Raises: + InvalidAttributeError: If any key in config_dict is not a valid + attribute of the graph database configuration. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cognee/api/v1/config/config.py
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: run_simple_example_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_multimedia_example_test / test
- GitHub Check: run_notebook_test / test
- GitHub Check: run_dynamic_steps_example_test / test
- GitHub Check: test
- GitHub Check: 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: docker-compose-test
- GitHub Check: profiler
🔇 Additional comments (1)
cognee/api/v1/config/config.py (1)
139-142
: Consider validating the configuration values.The method sets configuration values directly without any validation. Consider adding value validation similar to other config methods (e.g.,
set_graphistry_config
).Let me check what attributes are available in the graph config:
@staticmethod | ||
def set_graph_db_config(config_dict: dict) -> None: | ||
""" | ||
Updates the graph db config with values from config_dict. | ||
""" | ||
graph_db_config = get_graph_config() | ||
for key, value in config_dict.items(): | ||
if hasattr(graph_db_config, key): | ||
object.__setattr__(graph_db_config, key, value) | ||
else: | ||
raise AttributeError(message=f"'{key}' is not a valid attribute of the config.") | ||
|
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.
Maintain consistency with error handling across config methods.
The method uses AttributeError
while similar methods (set_llm_config
, set_relational_db_config
, set_vector_db_config
) use InvalidAttributeError
. This inconsistency could lead to different error handling flows.
Apply this diff to maintain consistency:
- raise AttributeError(message=f"'{key}' is not a valid attribute of the config.")
+ raise InvalidAttributeError(
+ message=f"'{key}' is not a valid attribute of the config."
+ )
Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
Bug Fixes