Skip to content
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

Parsing #5

Merged
merged 56 commits into from
Aug 24, 2024
Merged

Parsing #5

merged 56 commits into from
Aug 24, 2024

Conversation

dhirenmathur
Copy link
Contributor

No description provided.

@@ -21,6 +21,7 @@

# Construct the database URL from environment variables
POSTGRES_SERVER = os.getenv("POSTGRES_SERVER", "localhost")
print(POSTGRES_SERVER)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kindly remove this


load_dotenv()

neo4j_config = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make this a config provider class please

from app.modules.projects.projects_model import Project
from datetime import datetime

# User CRUD operations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not aligned with the structure of this file, has scope of improvements

  1. user , projects related stuff sits here , instead of their respective modules
  2. this is not a class.

The objective of this code feels similar to what can sit in a services file.




class SessionManager:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed? we already use get_db() to provide sessions and let them flow via router via dependency injection, example

async def get_conversation_info(
conversation_id: str,
db: Session = Depends(get_db)
):
controller = ConversationController(db)
return await controller.get_conversation_info(conversation_id)

import json
import logging

def firebase_init():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class please

app/models.py Outdated
@@ -0,0 +1,19 @@
from sqlalchemy.orm import relationship
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file is not needed, we removed this in my pr before merging, please remove and test.

@@ -0,0 +1,42 @@
from sqlalchemy.orm import Session
from typing import List, Optional
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundant, was renamed to chat_history_service, please compare to main and check

from app.core.mongo_manager import MongoManager

router = APIRouter()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be structured in a class

from app.modules.key_management.secret_manager import get_secret
from app.core.mongo_manager import MongoDBHelper

def get_llm_client(user_id, model_name):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have this part of a class as well

from sqlalchemy.orm import class_mapper
from sqlalchemy.orm.exc import DetachedInstanceError

def model_to_dict(model, max_depth=1, current_depth=0):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class

@vineetshar vineetshar self-requested a review August 23, 2024 11:34
@@ -0,0 +1,25 @@
import os
import firebase_admin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also this file should sit inside utils module

Copy link
Contributor

@vineetshar vineetshar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vineetshar vineetshar merged commit b038acb into main Aug 24, 2024
@vineetshar vineetshar deleted the parsing branch August 24, 2024 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants