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

Support CORS for openai api server #481

Merged
merged 3 commits into from
Oct 9, 2023

Conversation

aisensiy
Copy link
Contributor

Thanks for your contribution and we appreciate it a lot. The following instructions would make your pull request more healthy and more easily receiving feedbacks. If you do not understand some items, don't worry, just make the pull request and seek help from maintainers.

Motivation

Support CORS for openai api.

Modification

Add CORS headers.

BC-breaking (Optional)

Use cases (Optional)

Checklist

  1. Pre-commit or other linting tools are used to fix the potential lint issues.
  2. The modification is covered by complete unit tests. If not, please add more unit tests to ensure the correctness.
  3. If the modification has a dependency on downstream projects of a newer version, this PR should be tested with all supported versions of downstream projects.
  4. The documentation has been modified accordingly, like docstring or example tutorials.

@lvhan028
Copy link
Collaborator

@aisensiy could you please explain more about the motivation?
I

@aisensiy
Copy link
Contributor Author

CORS (Cross-Origin Resource Sharing) is a security mechanism implemented by web browsers to protect users from unauthorized access to their data. It allows web pages running on different domains to interact with each other. By adding CORS support, it enables this api to respond to requests from other domains.

For example this project running on http://localhost:8080 and an other web application running on http://localhost:3000. Calling this openai api from http://localhost:3000 will show an error message without the CORS configuration.

Access to XMLHttpRequest at 'http://localhost:8080/v1/chat/completions' from origin 'http://localhost:3000' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource.

@lvhan028
Copy link
Collaborator

Shouldn't the server who is listening to 3000 port deal with it?

@aisensiy
Copy link
Contributor Author

aisensiy commented Sep 27, 2023

No, the api provider should handle it. The api provider provide the CORS headers to tell others which origin can access the api.

cors-error.mp4

Please read more information about this in https://fastapi.tiangolo.com/tutorial/cors/

@aisensiy
Copy link
Contributor Author

@AllentDan @lvhan028 So there is still some concerns about this PR?

@AllentDan
Copy link
Collaborator

AllentDan commented Sep 28, 2023

Can we make it an option just like FastChat?
Maybe we can provide more args to launch api_server.

@tpoisonooo
Copy link
Collaborator

@aisensiy Excellent, please make it as an option, enable CORS by default.

Copy link
Collaborator

@tpoisonooo tpoisonooo left a comment

Choose a reason for hiding this comment

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

make it optional, enable CORS by default.

Copy link
Collaborator

@AllentDan AllentDan left a comment

Choose a reason for hiding this comment

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

LGTM

@lvhan028 lvhan028 merged commit 0268414 into InternLM:main Oct 9, 2023
1 check passed
@lvhan028 lvhan028 added the enhancement New feature or request label Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants