Adds anthropic /v1/messages endpoint to openai api_server #27882
Conversation
Signed-off-by: bbartels <[email protected]>
bbartels
requested review from
DarkLight1337,
NickLucche,
aarnphm,
chaunceyjiang,
robertgshaw2-redhat and
simon-mo
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new endpoint to support Anthropic's Messages API, which is a valuable addition. The implementation involves creating a new API endpoint and a serving layer to translate between Anthropic and OpenAI protocols, along with corresponding tests. My review has identified a few critical and high-severity issues that should be addressed. These include a bug in the tests that prevents a test case from running, a potential server crash due to a null reference in the new API endpoint, incorrect error response formatting, and missing error handling. I've provided detailed comments and suggestions for each of these points.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
bbartels
changed the title
Signed-off-by: bbartels <[email protected]>
Signed-off-by: bbartels <[email protected]>
Signed-off-by: bbartels <[email protected]>
Signed-off-by: bbartels <[email protected]>
Signed-off-by: bbartels <[email protected]>
Signed-off-by: bbartels <[email protected]>
Signed-off-by: bbartels <[email protected]>
Signed-off-by: bbartels <[email protected]>
|
/gemini review |
Signed-off-by: bbartels <[email protected]>
There was a problem hiding this comment.
Code Review
This pull request adds support for the Anthropic /v1/messages endpoint to the OpenAI-compatible API server. The implementation cleverly reuses the existing chat completion logic by creating an adapter layer that translates requests and responses between Anthropic and OpenAI formats. The changes to the API server and the addition of the serving logic are well-structured. However, the newly added test file introduces significant code duplication, which could pose maintenance challenges. I've provided a suggestion to refactor the tests for better maintainability.
|
Ready for review, not sure if we should just get rid of the separate anthropic |
Signed-off-by: bbartels <[email protected]>
Signed-off-by: Benjamin Bartels <[email protected]>
There was a problem hiding this comment.
I think this change is OK for now, but in the long term, the code becomes less maintainable as more APIs need to be supported. It is not ideal that the "OpenAI" server includes code from other APIs.
After this PR, I propose restructuring the code for online serving into the following modules:
vllm.entrypoints.serve.core: Contains the code for setting up the async client and FastAPI app. May include some common APIs such as health check.vllm.entrypoints.serve.openai: Contains only the code for OpenAI endpoints (e.g. Completions API, Chat Completions API, Responses API)vllm.entrypoints.serve.anthropic: Contains only the code for Anthropic endpoints (Messages API)vllm.entrypoints.serve.jina: Contains only the code for JinaAI endpoints (Reranker API)vllm.entrypoints.serve.vllm: Contains only the code for vLLM endpoints (e.g. Tokenize API, Pooling API, dev mode endpoints)
In vllm.entrypoints.serve, we can have the actual entrypoint which uses .core to build the server, then incrementally attach endpoints to the FastAPI app by importing relevant functions from the API-specific submodules.
|
I agree with @DarkLight1337, especially as we need to add other endpoints from the Anthropic api in order to work well with Claude Code such as /v1/messages/count_tokens
For now, I think we should land this addition and delete the explicit anthropic serve command and code. That code has only been in main for a week, so better to delete it quickly than introduce it into a release just to deprecate. Then open a PR/issue to restructure the code afterwards. |
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_anthropic_tool_call_streaming(client: anthropic.AsyncAnthropic): |
There was a problem hiding this comment.
In that case let's not copy the tests like this to avoid duplication in CI. We just need to check that the endpoint exists for OpenAI server since the same code is used to process the endpoint
Signed-off-by: bbartels <[email protected]>
|
In full agreement with you both :) Hence why i was rushing to make this change, seems more sustainable in the long run than to maintain two separate api servers. |
|
@mgoin @DarkLight1337 I have removed the old api server now, including the associated tests |
github-actions
bot
added
the
ready
Signed-off-by: bbartels <[email protected]>
Head branch was pushed to by a user without write access
…ct#27882) Signed-off-by: bbartels <[email protected]> Signed-off-by: Benjamin Bartels <[email protected]>
@DarkLight1337 I completely agree — I’ll give the refactor a try. #28040 |
…ct#27882) Signed-off-by: bbartels <[email protected]> Signed-off-by: Benjamin Bartels <[email protected]>
…ct#27882) Signed-off-by: bbartels <[email protected]> Signed-off-by: Benjamin Bartels <[email protected]>
5 participants
Purpose
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.