-
Notifications
You must be signed in to change notification settings - Fork 366
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
Open
revit13
wants to merge
24
commits into
llm-d:main
Choose a base branch
from
revit13:parser3
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
4d27912
refactor: unify API type handling across sidecar and coordinator
revit13 7fb3187
refactor: address simplify review findings
revit13 bf634a3
fix(request): cap max_output_tokens on Responses prefill legs
revit13 c9473b1
refactor(sidecar): cap the NIXLv2 prefill leg on a copy
revit13 121b1a2
refactor(sidecar): copy request bodies with maps.Clone
revit13 fdaa9a9
fix(sidecar): refuse request bodies that are not JSON objects
revit13 1738b06
test(request): cover a malformed sampling_params on the generate cap
revit13 e55885e
Add tests and simplify.
revit13 0545789
Merge origin/main into parser3
revit13 ba07e18
Merge branch 'main' into parser3
revit13 0f22ecc
Merge branch 'main' into parser3
revit13 359483f
Address review comments.
revit13 703e90c
Merge branch 'main' into parser3
revit13 320c820
Address review comments.
revit13 2bde31a
Merge branch 'main' into parser3
revit13 511fb6f
Default unrecognized API types to chat completions
revit13 09a1ab7
Clean up API type comments and tighten min_tokens prefill test
revit13 3313219
Remove LookupAPIType and use DetectAPIType in coordinator steps
revit13 54dd89f
State token-limit invariants once and reference them elsewhere
revit13 7b27bbf
Address review comments on API types and path constants
revit13 47692b1
Merge branch 'main' into parser3
revit13 de329aa
Address review comments on the EC encoder request
revit13 12bb81d
Use "request" instead of "leg" in comments and test names
revit13 27403ff
Merge branch 'main' into parser3
revit13 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 { | ||
| case APITypeCompletions: | ||
| return PathCompletions | ||
| case APITypeResponses: | ||
| return PathResponses | ||
| 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 | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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) | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.