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.

Sign up for GitHub

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

Jump to bottom

switch to aiohttp post request mode #2273

Merged
merged 1 commit into from
Aug 23, 2023
Merged

switch to aiohttp post request mode #2273

merged 1 commit into from
Aug 23, 2023

Conversation

leiwen83
Copy link
Contributor

Why are these changes needed?

httpx performance is very poor in fastapi. use aiohttp is much better

Related issue number (if applicable)

#2256

Checks

  • I've run format.sh to lint the changes in this PR.
  • I've included any doc changes needed.
  • I've made sure the relevant tests are passing (if applicable).

@merrymercy
Copy link
Member

Thanks for the contribution!

  1. Could you fix the lint errors?
  2. Could you run these tests? https://github.com/lm-sys/FastChat/blob/main/docs/commands/test_process.md#test-openai-api-server

@leiwen83 leiwen83 force-pushed the performance_httpx branch 2 times, most recently from 607a762 to cd1a262 Compare August 21, 2023 08:59
@leiwen83
Copy link
Contributor Author

Thanks for the contribution!

  1. Could you fix the lint errors?
  2. Could you run these tests? https://github.com/lm-sys/FastChat/blob/main/docs/commands/test_process.md#test-openai-api-server

Both 1/2 are done.

merrymercy
merrymercy requested changes Aug 23, 2023
View reviewed changes
fastchat/serve/openai_api_server.py Outdated
@@ -11,6 +11,7 @@
import argparse
import json
import logging
import httpx
Copy link
Member

Choose a reason for hiding this comment

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

Why is this still required?

Copy link
Member

Choose a reason for hiding this comment

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

I found one remaining usage of httpx. Should this be updated?
https://github.com/lm-sys/FastChat/blob/cbbd682692f98118574f3f463d617c3515301a86/fastchat/serve/openai_api_server.py#L580

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found one remaining usage of httpx. Should this be updated?

https://github.com/lm-sys/FastChat/blob/cbbd682692f98118574f3f463d617c3515301a86/fastchat/serve/openai_api_server.py#L580

Here httpx is used as stream, which is a different usage with other place.
I am not sure which aiohttp api should be used here, and how to verify it...

Copy link
Member

@merrymercy merrymercy Aug 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Okay. httpx is not a built-in library. Please move it to the second block of import statements.

fastchat/serve/openai_api_server.py Outdated
@@ -19,7 +20,7 @@
from fastapi.middleware.cors import CORSMiddleware
from fastapi.responses import StreamingResponse, JSONResponse
from fastapi.security.http import HTTPAuthorizationCredentials, HTTPBearer
import httpx
import aiohttp
Copy link
Member

Choose a reason for hiding this comment

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

  1. Sort the packages alphabetically
  2. Declare this dependency in pyproject.toml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Sort the packages alphabetically
  2. Declare this dependency in pyproject.toml

Done

@merrymercy
Copy link
Member

Do you have any performance numbers?

@leiwen83
Copy link
Contributor Author

Do you have any performance numbers?

Hi,

please see the performance data below.
I have tested batching request 10/20/30/40 for comparing aiohttp and httpx.

request type \request batch 10 20 30 40
aiohttp 2.5s 4.5s 6.1s 8.5s
httpx 8s 17s 27s httpx.ConnectTimeout

@merrymercy merrymercy merged commit 9c674ab into lm-sys:main Aug 23, 2023
@merrymercy
Copy link
Member

@leiwen83 Thanks! It is merged

@freddyheppell freddyheppell mentioned this pull request Mar 17, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merrymercy merrymercy merrymercy requested changes

Assignees
No one assigned
Labels
high-priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@leiwen83 @merrymercy @wenlei03