Simplify createChatCompletion #673
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR refactors the LLM client interface to provide more intuitive and specialized methods for different use cases. Here's a summary of the key changes:
- Split
createChatCompletioninto three focused methods:generateText,generateObject, andstreamTextacross all LLM clients - Added new example
llm_usage_wordle.tsdemonstrating usage of these simplified methods - Added streaming support through
createChatCompletionStreamimplementation in all clients - Standardized response formats to include both
dataandresponsefields for better data access - Implemented comprehensive error handling, logging, and caching across new methods
Key points to review:
createChatCompletionStreamhas unimplemented TODOs for response transformation and validation in several clients- Inconsistent error handling patterns between external clients (langchain, aisdk) and core clients
- Missing tests for Cerebras implementation due to API key unavailability
- Debug console.log statements need cleanup in langchain client
- Response type safety needs improvement in some implementations
💡 (2/5) Greptile learns from your feedback when you react with 👍/👎!
11 file(s) reviewed, 25 comment(s)
Edit PR Review Bot Settings | Greptile
| schema: z.object({ | ||
| guess: z | ||
| .string() | ||
| .describe("The 5-letter word that would be the best guess"), | ||
| }), |
There was a problem hiding this comment.
logic: Schema should validate that guess is exactly 5 letters using .length(5) since this is for Wordle
| schema: z.object({ | |
| guess: z | |
| .string() | |
| .describe("The 5-letter word that would be the best guess"), | |
| }), | |
| schema: z.object({ | |
| guess: z | |
| .string() | |
| .length(5) | |
| .describe("The 5-letter word that would be the best guess"), | |
| }), |
| logger, | ||
| retries = 3, | ||
| }: CreateChatCompletionOptions): Promise<T> { | ||
| console.log(logger, retries); |
There was a problem hiding this comment.
style: Remove debug console.log statement
| console.log(logger, retries); |
| const response = await generateObject({ | ||
| model: this.model, | ||
| prompt: prompt, | ||
| schema: schema, | ||
| ...options, | ||
| }); |
There was a problem hiding this comment.
style: options are spread after required parameters, which could accidentally override them
| try { | ||
| // Log the generation attempt | ||
| logger({ | ||
| category: "anthropic", |
There was a problem hiding this comment.
logic: Category 'anthropic' is incorrect for LangchainClient - should be 'langchain'
| category: "anthropic", | |
| category: "langchain", |
| category: "anthropic", | ||
| message: "Text generation successful", | ||
| level: 2, |
There was a problem hiding this comment.
logic: Log message indicates 'Text generation' but this is object generation
| const objResponse = { | ||
| object: generatedObject, | ||
| finishReason: response.response.choices[0].finish_reason, | ||
| usage: response.response.usage, | ||
| ...response, | ||
| } as T; |
There was a problem hiding this comment.
logic: Spreading response after setting specific fields may override the object/finishReason/usage properties
| export interface LLMObjectResponse extends BaseResponse { | ||
| data: Record<string, unknown>; | ||
| usage: UsageMetrics; | ||
| response: LLMResponse; | ||
| } |
There was a problem hiding this comment.
logic: LLMObjectResponse includes both 'data' and 'response' fields which seem redundant since 'response' already contains the full LLM response. This could lead to data inconsistency.
| // Object Response type that can include LLM properties | ||
| export interface ObjectResponse extends BaseResponse { | ||
| object: string; | ||
| choices?: LLMChoice[]; | ||
| usage?: UsageMetrics; | ||
| } |
There was a problem hiding this comment.
logic: ObjectResponse makes usage and choices optional but doesn't include the 'data' field from LLMObjectResponse. This inconsistency between response types could cause confusion.
| // TODO: O1 models - parse the tool call response manually and add it to the response | ||
| // TODO: Response model validation | ||
| // TODO: Caching |
There was a problem hiding this comment.
logic: Missing implementation for tool calls, response validation, and caching in streaming mode could cause issues with O1 models
| !response.data || | ||
| response.data.length === 0 || | ||
| response.data == undefined |
There was a problem hiding this comment.
logic: response.data.length check is unnecessary since response.data is an object, not an array
| !response.data || | |
| response.data.length === 0 || | |
| response.data == undefined | |
| !response.data || | |
| response.data === null || | |
| response.data === undefined |
1 participant
why
Simplified
createChatCompletionintogenerateText,generateObjectandstreamTextsimilar to Vercel AI SDK.what changed
Added
generateText,generateObjectandstreamTextfunctions toLLMClientfor all supported providers -OpenAI,Anthropic,Groq,Google,Cerebrasand external clients -aisdk,customOpenAI,langchaintest plan
Tested
generateText,generateObjectandstreamTextwith:OpenAIAnthropicGroqGoogleaisdkcustomOpenAIUnable to test for
Cerebrasdue to unavailability of API key