-
Notifications
You must be signed in to change notification settings - Fork 59
Add refresh_ontology() method to kg #126
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
WalkthroughPromotes the ontology schema graph to an instance attribute (self.ontology_graph), updates load/save paths to use it, and adds KnowledgeGraph.refresh_ontology() to reload ontology from the schema graph. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant KnowledgeGraph
participant SchemaGraph as ontology_graph
participant Ontology
rect rgba(230,240,255,0.5)
note over KnowledgeGraph: Initialization
Client->>KnowledgeGraph: __init__(schema_graph, ...)
KnowledgeGraph->>KnowledgeGraph: self.ontology_graph = schema_graph
KnowledgeGraph->>Ontology: from_schema_graph(self.ontology_graph)
Ontology-->>KnowledgeGraph: ontology
KnowledgeGraph->>KnowledgeGraph: self._ontology = ontology
end
rect rgba(235,255,235,0.5)
note over KnowledgeGraph: Refresh ontology
Client->>KnowledgeGraph: refresh_ontology()
KnowledgeGraph->>Ontology: from_schema_graph(self.ontology_graph)
Ontology-->>KnowledgeGraph: ontology
KnowledgeGraph->>KnowledgeGraph: self._ontology = ontology
end
rect rgba(255,245,230,0.5)
note over KnowledgeGraph: Save ontology
Client->>KnowledgeGraph: save (implicit/elsewhere)
KnowledgeGraph->>Ontology: save_to_graph(self.ontology_graph)
Ontology-->>KnowledgeGraph: saved
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
graphrag_sdk/kg.py (1)
193-201: Also delete the schema/ontology graph to avoid orphaned resources.Now that
ontology_graphis a first-class attribute,delete()should clean it up alongside the data graph; otherwise,{name}_schemaremains in the DB.Proposed change:
# Delete KnowledgeGraph if self.name in available_graphs: self.graph.delete() + # Delete Ontology/Schema graph + schema_graph_name = f"{{{self._name}}}_schema" + if schema_graph_name in available_graphs: + self.ontology_graph.delete() + # Nullify all attributes for key in self.__dict__.keys(): setattr(self, key, None)
🧹 Nitpick comments (2)
graphrag_sdk/kg.py (2)
64-65: Centralize schema graph naming and confirm Redis Cluster hashtag intent.Good move introducing a dedicated schema graph handle. To avoid hardcoding and to reuse the same name elsewhere (e.g., deletion), extract the naming into a helper or attribute. Also, please confirm the use of curly braces is intentional for Redis Cluster hashtagging; if users pass names that already contain braces, the key could become awkward.
Proposed minimal change:
- self.ontology_graph = self.db.select_graph("{" + name + "}" + "_schema") + self._schema_graph_name = f"{{{name}}}_schema" + self.ontology_graph = self.db.select_graph(self._schema_graph_name)Optional helper to normalize user-provided names with/without braces:
+ def _schema_graph_name_for(self, base: str) -> str: + # Ensure Redis Cluster hashtag for co-location, even if user supplied braces. + if base.startswith("{") and base.endswith("}"): + tag = base[1:-1] + else: + tag = base + return f"{{{tag}}}_schema"
214-228: refresh_ontology: docstring vs. behavior mismatch — choose one and align.The docstring says it may raise if the refreshed ontology is empty, but the implementation always overwrites, even with an empty ontology. Pick one behavior and codify it.
Option A — enforce non-empty refresh (raise and keep the old ontology):
def refresh_ontology(self) -> None: @@ - Raises: - Exception: If the refreshed ontology is empty and no fallback is available. + Raises: + Exception: If the refreshed ontology is empty. @@ - refreshed_ontology = Ontology.from_schema_graph(self.ontology_graph) - - # Always update the ontology, even if it's empty - # This allows users to intentionally clear the ontology if needed - self._ontology = refreshed_ontology + refreshed_ontology = Ontology.from_schema_graph(self.ontology_graph) + if len(refreshed_ontology.entities) == 0 and len(refreshed_ontology.relations) == 0: + raise Exception("The refreshed ontology is empty; refusing to overwrite the current ontology.") + self._ontology = refreshed_ontologyOption B — allow empty refreshes (update docstring to reflect current behavior):
def refresh_ontology(self) -> None: @@ - Raises: - Exception: If the refreshed ontology is empty and no fallback is available. + Note: + If the refreshed ontology is empty, the current ontology will be replaced + with an empty ontology. This allows intentionally clearing the ontology.Would you like me to add unit tests to validate both paths (non-empty refresh and empty refresh)? I can scaffold tests around mocked
Ontology.from_schema_graph.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
graphrag_sdk/kg.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
graphrag_sdk/kg.py (1)
graphrag_sdk/ontology.py (3)
Ontology(35-404)from_schema_graph(104-125)save_to_graph(389-404)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (openai/gpt-4.1)
🔇 Additional comments (1)
graphrag_sdk/kg.py (1)
66-76: LGTM: load/save now correctly target the instance-level schema graph.Using
Ontology.from_schema_graph(self.ontology_graph)on load andontology.save_to_graph(self.ontology_graph)on save keeps concerns separated between data graph and schema graph. The emptiness check on first load remains intact.
Summary by CodeRabbit
New Features
Refactor