Knowledge backend #737
Knowledge backend #737
Conversation
Knowledge backend
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## dev #737 +/- ##
==========================================
- Coverage 58.49% 56.18% -2.31%
==========================================
Files 146 159 +13
Lines 5847 6491 +644
Branches 604 660 +56
==========================================
+ Hits 3420 3647 +227
- Misses 2284 2701 +417
Partials 143 143
☔ View full report in Codecov by Sentry. |
| depends_on = None | ||
|
|
||
|
|
||
| def upgrade() -> None: |
There was a problem hiding this comment.
why do u need this migration?
| depends_on = None | ||
|
|
||
|
|
||
| def upgrade() -> None: |
There was a problem hiding this comment.
why this migration?
| sa.Column('updated_at', sa.DateTime(), nullable=True), | ||
| sa.PrimaryKeyConstraint('id') | ||
| ) | ||
| op.create_table('marketplace_state', |
There was a problem hiding this comment.
dont even create this table if u are dropping later.
There was a problem hiding this comment.
table name should be knowledges, vector_dbs
|
|
||
| def upgrade() -> None: | ||
| # ### commands auto generated by Alembic - please adjust! ### | ||
| op.create_table('knowledge', |
There was a problem hiding this comment.
all tables names should be plural.
|
|
||
| def upgrade() -> None: | ||
| # ### commands auto generated by Alembic - please adjust! ### | ||
| op.create_table('knowledge_config', |
There was a problem hiding this comment.
make it knowledge_configs
| # ### commands auto generated by Alembic - please adjust! ### | ||
| op.create_table('knowledge_config', | ||
| sa.Column('id', sa.Integer(), autoincrement=True, nullable=False), | ||
| sa.Column('knowledge_id', sa.Integer(), nullable=True), |
There was a problem hiding this comment.
id, knowledge_id, key cant be nullable.
| sa.Column('updated_at', sa.DateTime(), nullable=True), | ||
| sa.PrimaryKeyConstraint('id') | ||
| ) | ||
| op.create_table('vector_db_index_collection', |
There was a problem hiding this comment.
should be vector_db_indexes. index_collection does not make sense.
name, vector_db_id can't be nullable
| router = APIRouter() | ||
|
|
||
|
|
||
| @router.get("/get/list") |
There was a problem hiding this comment.
marketplace/list
| @router.get("/user/list") | ||
| def get_user_knowledge_list(Authorize: AuthJWT = Depends(), organisation = Depends(get_user_organisation)): | ||
| organisation_id = organisation.id | ||
| marketplace_organisation_id = int(get_config("MARKETPLACE_ORGANISATION_ID")) |
There was a problem hiding this comment.
logic does not seem right. local one cant even fetch MARKETPLACE_ORGANISATION_ID. In local nobody sets MARKETPLACE_ORGANISATION_ID
| from superagi.helper.auth import get_user_organisation | ||
| router = APIRouter() | ||
| @router.get("/get/list") | ||
| def handle_marketplace_operations_list(): |
There was a problem hiding this comment.
rename the method.
| pinecone = [] | ||
| qdrant = [] | ||
| for vector in vector_dbs: | ||
| indices = db.session.query(VectorIndexCollection).filter(VectorIndexCollection.vector_db_id == vector["id"]).all() |
There was a problem hiding this comment.
not a right pattern to call sql inside a loop
| qdrant = [] | ||
| for vector in vector_dbs: | ||
| indices = db.session.query(VectorIndexCollection).filter(VectorIndexCollection.vector_db_id == vector["id"]).all() | ||
| for index in indices: |
There was a problem hiding this comment.
not a right pattern to call sql inside a loop
| from superagi.models.knowledge_config import KnowledgeConfig | ||
| from superagi.models.index_config import VectorIndexConfig | ||
|
|
||
| class KnowledgeHelper: |
There was a problem hiding this comment.
helper classes does not make sense. Create a new package knowledge and restructure the classes.
|
|
||
| def upgrade() -> None: | ||
| # ### commands auto generated by Alembic - please adjust! ### | ||
| op.create_table('vector_index_config', |
There was a problem hiding this comment.
rename to vector_index_configs.
| added_indices = new_indices_names - existing_indices_names | ||
| for index in added_indices: | ||
| vector_index = VectorIndexCollection.add_vector_index(db.session, existing_index, vector_db_id) | ||
| if vector_db.db_type == "Pinecone": |
There was a problem hiding this comment.
We cant have if else for each db type can we have better structure.
| from superagi.models.vector_db_config import VectordbConfig | ||
| from qdrant_client import models, QdrantClient | ||
|
|
||
| class QdrantHelper: |
There was a problem hiding this comment.
We already have these vector dbs in the vector_dbs. These classes does not make sense.
|
|
||
| @classmethod | ||
| def delete_vector_index_config(cls, session, vector_index_id): | ||
| session.query(VectorIndexConfig).filter(VectorIndexConfig.vector_index_id == vector_index_id).delete() |
There was a problem hiding this comment.
if no element found delete might throw exception
| knowledge_details = dbhelper.get_knowledge_details(knowledge) | ||
| query_knowledge = KnowledgeToolHelper() | ||
|
|
||
| if knowledge_details["knowledge_vector_db_type"] == "Pinecone": |
There was a problem hiding this comment.
Get a better way than if else
| description = Column(String) | ||
| summary = Column(String) | ||
| readme = Column(String) | ||
| index_id = Column(Integer) |
There was a problem hiding this comment.
vector_db_index_id
| from superagi.models.base_model import DBBaseModel | ||
|
|
||
|
|
||
| class VectorIndexConfig(DBBaseModel): |
There was a problem hiding this comment.
clean up the file_name.
There was a problem hiding this comment.
vector_db_index_configs
Description
Related Issues
Solution and Design
Test Plan
Type of change
Checklist