Conversation
There was a problem hiding this comment.
Code Review
This pull request implements support for the legacy OpenAI completions API, adding new routes, model definitions, and specialized transformers. A significant architectural change updates the Outbound transformer interface to pass the original request to streaming and aggregation methods, enabling better routing for composite transformers. Review feedback points out a typo in the DeepSeek completion URL configuration and identifies logic errors in the completion stream aggregator, which incorrectly merges multiple response choices and fails to return the full set of aggregated choices.
I am having trouble creating individual review comments. Click here to see my feedback.
llm/transformer/deepseek/outbound.go (57)
The URL suffix # seems to be a typo for ##. In this repository, ## is used as a convention to indicate a raw URL that should not be further normalized or modified by the base transformer. Additionally, since DeepSeek's beta completion endpoint is /beta/completions, and the CompletionOutboundTransformer appends /completions by default unless RawURL is true, you should use beta/completions## to ensure the correct endpoint is called.
BaseURL: strings.TrimSuffix(baseURL, "/v1") + "/beta/completions##",
llm/transformer/openai/completion_aggregator.go (52-57)
The current aggregation logic incorrectly merges text from all choices into a single string. OpenAI completions can return multiple choices (when n > 1), and their content must be accumulated separately based on their index. Interleaving chunks for different choices is possible in streaming.
for _, choice := range compResp.Choices {
if _, ok := accumulatedChoices[choice.Index]; !ok {
accumulatedChoices[choice.Index] = &llm.CompletionChoice{Index: choice.Index}
}
accumulatedChoices[choice.Index].Text += choice.Text
if choice.FinishReason != nil {
accumulatedChoices[choice.Index].FinishReason = choice.FinishReason
}
}llm/transformer/openai/completion_aggregator.go (82-89)
The response should include all aggregated choices, sorted by their index, rather than a single hardcoded choice.
Completion: &llm.CompletionResponse{
Choices: lo.Map(lo.Values(accumulatedChoices), func(c *llm.CompletionChoice, _ int) llm.CompletionChoice {
if c.FinishReason == nil {
c.FinishReason = lo.ToPtr("stop")
}
return *c
}),
},
Greptile SummaryThis PR adds an OpenAI-compatible legacy completions API ( Confidence Score: 5/5Safe to merge; DeepSeek completions work correctly and all remaining findings are P2 quality/edge-case items. All four findings are P2: N>1 aggregation edge case, string-only prompt limitation, TransformError duplication, and missing default endpoint for TypeOpenai. None cause data loss or silent wrong results for the feature's primary target (DeepSeek). The interface change is broad but mechanically consistent across all implementations. llm/transformer/openai/completion_aggregator.go (N>1 choice handling), internal/server/biz/channel_endpoint.go (openAICompatibleDefaultEndpoints gap) Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Router as POST /v1/completions
participant InboundT as CompletionInboundTransformer
participant Orch as Orchestrator
participant DS as DeepSeek OutboundTransformer
participant CompT as CompletionOutboundTransformer
participant Provider as DeepSeek API /beta/completions
Client->>Router: POST /v1/completions {model, prompt, ...}
Router->>InboundT: TransformRequest(httpReq)
InboundT-->>Orch: llm.Request{RequestType=completion, Completion={...}}
Orch->>DS: TransformRequest(llmReq)
DS->>CompT: route via RequestTypeCompletion
CompT-->>Orch: httpclient.Request{URL=/beta/completions}
Orch->>Provider: POST /beta/completions
Provider-->>Orch: CompletionResponse (or stream)
alt Non-streaming
Orch->>DS: TransformResponse(httpResp)
DS->>CompT: route via httpResp.Request.RequestType
CompT-->>Orch: llm.Response{Completion={...}}
Orch->>InboundT: TransformResponse(llmResp)
InboundT-->>Client: CompletionResponse JSON
else Streaming
Orch->>DS: TransformStream(req, stream)
DS->>CompT: route via req.RequestType
CompT-->>Orch: Stream[llm.Response]
Orch->>InboundT: TransformStream(llmStream)
InboundT-->>Client: SSE stream of CompletionResponse chunks
end
|
No description provided.