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

Fastapi Logic for Pippy Fixes#1 #12

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kshitijdshah99
Copy link
Contributor

@kshitijdshah99 kshitijdshah99 commented Jan 2, 2025

@walterbender @chimosky This includes the FastApi framework for the RAG model of Pippy's AI assistant Fixes#1

@chimosky
Copy link
Member

chimosky commented Jan 2, 2025

I mentioned you in #9, I'm guessing you haven't taken a look, please do and make the necessary changes to that PR as there's some changes there already.

Your use of if __name__ == "__main__" isn't great here, take a look at my mention and review the PR.

@kshitijdshah99
Copy link
Contributor Author

Yes, his implemented code misses the LCEL chain and some other functionalities. I'll make those changes soon.

@kshitijdshah99
Copy link
Contributor Author

@chimosky , can you tell me if any other modification is required in this code? Then, I can open a PR to the Qixiang Branch.

pippy_fastapi.py Outdated Show resolved Hide resolved
pippy_fastapi.py Outdated Show resolved Hide resolved
pippy_fastapi.py Outdated Show resolved Hide resolved
pippy_fastapi.py Outdated Show resolved Hide resolved
agent.setup_vectorstore(document_paths)

# Define API routes
@app.get("/")
@router.get("/")
Copy link
Member

Choose a reason for hiding this comment

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

Does each router require it's own root route?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, each router does not require its root route. I've added it to provide a default response. It acts as an entry point for our API.

Copy link
Member

Choose a reason for hiding this comment

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

Would they be a need to provide a default response seeing as when pointing the activity to it, you'll use the right url.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok so shall I replace it with something like /Pippy_AI_Assistant ?

pippy_fastapi.py Outdated Show resolved Hide resolved
pippy_fastapi.py Outdated Show resolved Hide resolved
@chimosky
Copy link
Member

chimosky commented Jan 6, 2025

@chimosky , can you tell me if any other modification is required in this code? Then, I can open a PR to the Qixiang Branch.

I've made some comments, please write better commit messages, "Modified Fastapi code" isn't a great commit message and tells very little about the changes you made in the commit.

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