-
-
Notifications
You must be signed in to change notification settings - Fork 135
Llm flask api #1789
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
base: main
Are you sure you want to change the base?
Llm flask api #1789
Conversation
- Add basic incomplete post body testing. Running them for real would require OpenAI calls
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Nathan's comments from the Zoom on cutting down dependency: I expect the imports here are sufficient to run the python llm functions: Line 26 in 8032ba1
There are some fairly hefty ones there that are needed... langchain, sklearn, chromadb We can probably cut sklearn if we find easy replacement for the small number of functions we are importing from it. |
if summary["status"] in [-1, -2]: | ||
abort(500, description="Unable to generate summary") |
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.
The status field returns either -1 or -2 if it fails... This is probably somewhat unfortunate here because if this API ever changes I will not be able to know about it.
I'll add a note to get_summary_api_function
but we don't really have types.
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.
Or, maybe I should just check that this value is negative? technically the return type is int
if tags["status"] in [-1, -2]: | ||
abort(500, description="Unable to generate tags") |
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.
Probably check for negative return type? See above https://github.com/codeforboston/maple/pull/1789/files#r2055086855
Add a GET /ready endpoint for testing Add some command examples to readme.md
"predeploy": [". llm/venv/bin/activate && python3 -m pip install -r llm/requirements.txt"], | ||
"source": "llm", | ||
"codebase": "maple-llm", | ||
"runtime": "python311" |
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.
This is the version running inside Dockerfile.firebase
, and this is actually required to build the virtual environment see llm/readme.md
for more information.
@@ -1,183 +1,16 @@ | |||
absl-py==2.1.0 |
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.
Minimized and simplified the requirements file.
@@ -4,11 +4,20 @@ | |||
"cleanUrls": true, |
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.
Note: this PR requires an update to firebase-tools
in package.json
!
|
||
# Running the API | ||
|
||
Set up a virtual environment and run the Flask app |
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.
Conceptually, there are a few ways you can run this application.
- Locally with no dependencies on firebase, that is the first set of notes
- Deployed as a function with firebase locally, that is the second set of notes
- Deploying to firebase directly
"2." is by far the hardest because we directly overlay /app
into the Docker.firebase
container in docker-compose.yaml
. So, we need the virtual env and python version when running inside the container to match! Therefore, either we stop overlaying /app
or we build the virtual environment from inside the container. We chose the latter because it is a simpler change.
@@ -278,6 +278,7 @@ def set_my_llm_cache(cache_file: Path=LLM_CACHE) -> SQLiteCache: | |||
Set an LLM cache, which allows for previously executed completions to be | |||
loaded from disk instead of repeatedly queried. | |||
""" | |||
cache_file.parent.mkdir(exist_ok=True) |
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.
This was required because the container doesn't have this path and we need it to build the LLM cache
@@ -1,7 +1,7 @@ | |||
FROM andreysenov/firebase-tools:latest-node-18 | |||
|
|||
USER root | |||
RUN apt update && apt install -y curl | |||
RUN apt update && apt install -y curl python3 python3-pip python3-venv |
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.
We need python in this container to build the virtual environments, see readme.md
|
||
|
||
def set_openai_api_key(): | ||
match os.environ.get("MAPLE_DEV"): |
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.
AFAICT, we use this to deploy dev versus prod. I re-use it here to select an environment. IIUC, we have a lost less dollars in OPENAI_DEV
so it seems like a safe default.
"deploy:backend:dev": "firebase --project=default deploy --only firestore,functions:maple,storage", | ||
"deploy:backend:prod": "firebase --project=prod deploy --only firestore,functions:maple,storage", |
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.
Now that we also have Python, and it is a little janky right now, just deploy the maple functions and skip the python ones.
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.
It kind of sucks that this is a JSON file... Obviously, not something you can control.
@app.route("/ready", methods=["GET"]) | ||
def ready(): | ||
return "" |
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.
This may not be necessary...
DO NOT MERGE
This PR requires an update to
firebase-tools
inpackage.json
. It at least needs to be"firebase-tools": "^13.23.0",
.Summary
In this PR, I introduce a wrapper for our LLM functions via an HTTP firebase function. It contains a bunch of infrastructure updates to support deploying the new Python firebase functions. The only very tricky part is deploying via our local docker compose pipeline and I tried to document the deficiencies in the process.
Checklist
Not a FE change.
Screenshots
N/A
Known issues
If you've run against limitations or caveats, include them here. Include follow-up issues as well.
Steps to test/reproduce
For each feature or bug fix, create a step by step list for how a reviewer can test it out. E.g.: