-
Notifications
You must be signed in to change notification settings - Fork 816
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
[candidate_isaac] opengpts: ingest progress bar and python eval tool #326
base: main
Are you sure you want to change the base?
Conversation
@@ -44,7 +48,27 @@ async def ingest_files( | |||
if thread is None: | |||
raise HTTPException(status_code=404, detail="Thread not found.") | |||
|
|||
return ingest_runnable.batch([file.file for file in files], config) | |||
if config["configurable"].get("show_progress_bar"): | |||
ingest_runnable.abatch_as_completed([file.file for file in files], config) |
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.
I assume this shouldn't be 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.
Yes sorry, this is a mistake.
@@ -44,7 +48,27 @@ async def ingest_files( | |||
if thread is None: | |||
raise HTTPException(status_code=404, detail="Thread not found.") | |||
|
|||
return ingest_runnable.batch([file.file for file in files], config) | |||
if config["configurable"].get("show_progress_bar"): |
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.
would maybe be better to branch based on request headers? eg the Accept
header
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.
Using the Accept
header here makes sense
yield orjson.dumps(x) | ||
await asyncio.sleep(3) # Debug: to demonstrate streaming | ||
|
||
return StreamingResponse( |
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.
ideally we'd use a more standard streaming format, eg Server Sent Events
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.
I'm not familiar with Server Sent Events, but just for completeness, what's the concern with fastapi.StreamingResponse()
?
df.write( | ||
string.Template(DOCKERFILE_TEMPLATE) | ||
.substitute( | ||
base_image="alpine", |
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.
ooc why not start from a python base image?
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.
Mostly that I am not familiar with a small-ish python base image. I chose alpine
because it looked minimal and efficient.
A couple of notes
git add --interactive
I'll skip those ;)In this diff
/ingest
endpoint on the OpenGPTs server, stream updates back to the caller as each file completes uploading./ingest
Considerations
The key change in this section is to return a
StreamingResponse
around theingest_runnable.abatch_as_completed
async generator. Inserver.py
:Test Plan
Open OpenGPTs frontend in browser. Navigate to existing Thread and upload multiple files along with a dummy message. Observe that progress information is printed via.
console.log
.Modify source to omit
show_progress_bar
on the client side, and notice how no progress info is printed.Modify source to use previous and new versions of server side to show that providing
show_progress_bar
to server which does not understand it, does not cause any changes.For a smaller test case, you can use the following script to make an
/ingest
request:Python Source
Considerations
The main tradeoff in this script is how the Docker container gets created. There are two basic approaches that come to mind:
For this diff, I implemented the ad-hoc strategy because we don't have a compelling reason to add the complexity of the full server approach, and the ad-hoc approach is portable and flexible.
Test Plan
Provide the following Python source in e.g.
test.py
, and observe that easy escalations toroot
are not possible.