-
Notifications
You must be signed in to change notification settings - Fork 383
refactor: unify API type handling across sidecar and coordinator #2743
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
Changes from 22 commits
4d27912
7fb3187
bf634a3
c9473b1
121b1a2
fdaa9a9
1738b06
e55885e
0545789
ba07e18
0f22ecc
359483f
703e90c
320c820
2bde31a
511fb6f
09a1ab7
3313219
54dd89f
7b27bbf
47692b1
de329aa
12bb81d
27403ff
c31bba2
a48a7de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| /* | ||
| Copyright 2026 The llm-d Authors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package request | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "strings" | ||
| ) | ||
|
|
||
| // Inference API paths served by the sidecar and the coordinator. | ||
| const ( | ||
| PathChatCompletions = "/v1/chat/completions" | ||
| PathCompletions = "/v1/completions" | ||
| PathResponses = "/v1/responses" | ||
| PathMessages = "/v1/messages" | ||
| PathGenerate = "/inference/v1/generate" | ||
| ) | ||
|
|
||
| // APIType is the inference API a request was sent to. A value outside the | ||
| // constants below degrades to APITypeChatCompletions. | ||
| type APIType int | ||
|
|
||
| const ( | ||
| // APITypeChatCompletions is the Chat Completions API (/v1/chat/completions). | ||
| APITypeChatCompletions APIType = iota | ||
| // APITypeCompletions is the legacy Completions API (/v1/completions). | ||
| APITypeCompletions | ||
| // APITypeResponses is the Responses API (/v1/responses). | ||
| APITypeResponses | ||
| // APITypeGenerate is vLLM's token-in generate API (/inference/v1/generate). | ||
| APITypeGenerate | ||
| // APITypeMessages is the Anthropic Messages API (/v1/messages). | ||
| APITypeMessages | ||
| ) | ||
|
|
||
| // String implements fmt.Stringer so structured logs show readable API names. | ||
| func (a APIType) String() string { | ||
| switch a { | ||
| case APITypeChatCompletions: | ||
| return "chat_completions" | ||
| case APITypeCompletions: | ||
| return "completions" | ||
| case APITypeResponses: | ||
| return "responses" | ||
| case APITypeGenerate: | ||
| return "generate" | ||
| case APITypeMessages: | ||
| return "messages" | ||
| default: | ||
| return fmt.Sprintf("APIType(%d)", int(a)) | ||
| } | ||
| } | ||
|
|
||
| // Path returns the canonical request path for the API. | ||
| func (a APIType) Path() string { | ||
| switch a { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion (not a change request): |
||
| case APITypeCompletions: | ||
| return PathCompletions | ||
| case APITypeResponses: | ||
| return PathResponses | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||
| case APITypeGenerate: | ||
| return PathGenerate | ||
| case APITypeMessages: | ||
| return PathMessages | ||
| default: | ||
| return PathChatCompletions | ||
| } | ||
| } | ||
|
|
||
| // DetectAPIType classifies a request path. An unrecognized path maps to | ||
| // APITypeChatCompletions: callers that route only known paths never reach the | ||
| // fallback. | ||
| func DetectAPIType(path string) APIType { | ||
| switch { | ||
| case strings.Contains(path, PathChatCompletions): | ||
| return APITypeChatCompletions | ||
| case strings.Contains(path, PathCompletions): | ||
| return APITypeCompletions | ||
| case strings.Contains(path, PathResponses): | ||
| return APITypeResponses | ||
| case strings.Contains(path, PathMessages): | ||
| return APITypeMessages | ||
| case strings.Contains(path, PathGenerate): | ||
| return APITypeGenerate | ||
| default: | ||
| return APITypeChatCompletions | ||
| } | ||
| } | ||
|
|
||
| // JSON request field names that cap output tokens, by API. Chat completions caps | ||
| // both max_tokens and max_completion_tokens: vLLM and SGLang accept the two | ||
| // together and prefer max_completion_tokens, so capping both bounds the request | ||
| // regardless of which field the engine consults. The Completions, Messages, and | ||
| // generate APIs share a list: none of them defines max_completion_tokens, so | ||
| // capping it would put a field on the wire that a strict server is free to | ||
| // reject. | ||
| var ( | ||
| chatCompletionTokenLimitFields = []string{FieldMaxTokens, FieldMaxCompletionTokens} | ||
| maxTokensOnlyTokenLimitFields = []string{FieldMaxTokens} | ||
| responsesTokenLimitFields = []string{FieldMaxOutputTokens} | ||
| ) | ||
|
|
||
| // tokenLimitFields returns the output token cap field names the API uses. | ||
| // The returned slices are shared package-level vars; callers must not mutate them. | ||
| func (a APIType) tokenLimitFields() []string { | ||
| switch a { | ||
| case APITypeResponses: | ||
| return responsesTokenLimitFields | ||
| case APITypeCompletions, APITypeGenerate, APITypeMessages: | ||
| return maxTokensOnlyTokenLimitFields | ||
| default: | ||
| return chatCompletionTokenLimitFields | ||
|
roytman marked this conversation as resolved.
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| /* | ||
| Copyright 2026 The llm-d Authors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package request | ||
|
|
||
| import ( | ||
| "reflect" | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestAPIType_StringAndPath(t *testing.T) { | ||
| cases := map[APIType]struct{ name, path string }{ | ||
| APITypeChatCompletions: {"chat_completions", PathChatCompletions}, | ||
| APITypeCompletions: {"completions", PathCompletions}, | ||
| APITypeResponses: {"responses", PathResponses}, | ||
| APITypeGenerate: {"generate", PathGenerate}, | ||
| APITypeMessages: {"messages", PathMessages}, | ||
| APIType(7): {"APIType(7)", PathChatCompletions}, | ||
| } | ||
| for apiType, want := range cases { | ||
| if got := apiType.String(); got != want.name { | ||
| t.Errorf("APIType(%d).String() = %q, want %q", int(apiType), got, want.name) | ||
| } | ||
| if got := apiType.Path(); got != want.path { | ||
| t.Errorf("APIType(%d).Path() = %q, want %q", int(apiType), got, want.path) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestDetectAPIType(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| path string | ||
| want APIType | ||
| }{ | ||
| {name: "chat completions", path: PathChatCompletions, want: APITypeChatCompletions}, | ||
| {name: "completions", path: PathCompletions, want: APITypeCompletions}, | ||
| {name: "responses", path: PathResponses, want: APITypeResponses}, | ||
| {name: "messages", path: PathMessages, want: APITypeMessages}, | ||
| {name: "generate", path: PathGenerate, want: APITypeGenerate}, | ||
| {name: "prefixed chat completions", path: "/prefix" + PathChatCompletions, want: APITypeChatCompletions}, | ||
| {name: "prefixed completions", path: "/prefix" + PathCompletions, want: APITypeCompletions}, | ||
| {name: "prefixed messages", path: "/prefix" + PathMessages, want: APITypeMessages}, | ||
| {name: "prefixed generate", path: "/prefix" + PathGenerate, want: APITypeGenerate}, | ||
| {name: "unknown path falls back to chat completions", path: "/v1/embeddings", want: APITypeChatCompletions}, | ||
| {name: "empty path falls back to chat completions", path: "", want: APITypeChatCompletions}, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| if got := DetectAPIType(tt.path); got != tt.want { | ||
| t.Errorf("DetectAPIType(%q) = %v, want %v", tt.path, got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestAPIType_tokenLimitFields(t *testing.T) { | ||
| cases := map[APIType][]string{ | ||
| APITypeChatCompletions: {FieldMaxTokens, FieldMaxCompletionTokens}, | ||
| APITypeCompletions: {FieldMaxTokens}, | ||
| APITypeResponses: {FieldMaxOutputTokens}, | ||
| APITypeGenerate: {FieldMaxTokens}, | ||
| APITypeMessages: {FieldMaxTokens}, | ||
| APIType(7): {FieldMaxTokens, FieldMaxCompletionTokens}, | ||
| } | ||
| for apiType, want := range cases { | ||
| if got := apiType.tokenLimitFields(); !reflect.DeepEqual(got, want) { | ||
| t.Errorf("APIType(%d).tokenLimitFields() = %v, want %v", int(apiType), got, want) | ||
| } | ||
| } | ||
| } |
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.
Two things worth a second look here, not blocking:
APITypeis a bareintwith no validating constructor, and the "degrades to chat completions" rule is repeated independently inPath,tokenLimitFields,tokenLimitMap, andDetectAPITyperather than centralized. There's also noexhaustivelint rule enabled for this package, so a 5thAPITypeconstant added later would compile cleanly and silently degrade in every one of these switches exceptprefill.go, which treats an unhandled value as a hard error. Worth enablingexhaustivefor this package if the closed-set invariant is meant to be enforced rather than conventional.(We can return to it later)
The PR description's test plan says unknown/out-of-range values "fall back to generate." They actually fall back to chat completions (confirmed by this file and by
apitype_test.go:56,74). Worth fixing the PR description so reviewers aren't misled about which API type is the actual default.