-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Cognee integration #17314
base: main
Are you sure you want to change the base?
Cognee integration #17314
Conversation
Proposal for integration of cognee to llama-index
Resolve issue of propagating llm key to cognee in test fix
Added more descriptions to cognee GraphRAG Refactor
Updated README.md docs
…into cognee-integration
Added lint and formatting changes Docs
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.
no need to commit the lock 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.
Removed lock file
graph_db_provider, | ||
vector_db_provider, | ||
relational_db_provider, | ||
db_name, |
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.
None of these args are typed
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.
Added typing
) | ||
cognee.config.system_root_directory(cognee_directory_path) | ||
|
||
async def add(self, data, dataset_name: str): |
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.
Can we add return type annotations, even its its -> None
?
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.
Yes, it's added
from .base import GraphRAG | ||
|
||
|
||
class CogneeGraphRAG(GraphRAG): |
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.
Maybe a docstring is needed here
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.
Added
from llama_index.graph_rag.cognee import CogneeGraphRAG | ||
|
||
|
||
async def test_graph_rag_cognee(): |
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.
These tests will not run in CICD. We probably need to
- mark this as skip if the appropriate API keys are missing
- maybe write tests that mock network calls? Up to you
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.
Added marking as skipped if API key is missing
Description
Integration of the cognee GraphRAG to LlamaIndex.
With added tests, README.md and usage example in README.md
Fixes # (issue)
New Package?
Did I fill in the
tool.llamahub
section in thepyproject.toml
and provide a detailed README.md for my new integration or package?Version Bump?
Did I bump the version in the
pyproject.toml
file of the package I am updating? (Except for thellama-index-core
package)Type of Change
Please delete options that are not relevant.
How Has This Been Tested?
Your pull-request will likely not be merged unless it is covered by some form of impactful unit testing.
Suggested Checklist:
make format; make lint
to appease the lint gods