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

switch to aiohttp post request mode #2273

Merged
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
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.

@@ -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?

async with httpx.AsyncClient() as client:

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?

async with httpx.AsyncClient() as client:

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

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.

@@ -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

Signed-off-by: Lei Wen <wenlei03@qiyi.com>
@merrymercy merrymercy merged commit 9c674ab into lm-sys:main Aug 23, 2023
1 check passed
@merrymercy
Copy link
Member

@leiwen83 Thanks! It is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants