Improve base llm service #104
Conversation
🦋 Changeset detectedLatest commit: 43a1c38 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds streaming support to the LLM service, enabling real-time text generation responses from language models. The implementation adds two new optional methods to handle streaming use cases with and without tool support.
- Added
runStreamedLLMandrunStreamedLLMWithToolsmethods for streaming text generation - Updated type definitions to include optional streaming method signatures
- Bumped package version from 0.35.0 to 0.36.0
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 14 comments.
| File | Description |
|---|---|
| packages/core/src/types/llm.types.ts | Added optional method signatures for runStreamedLLM and runStreamedLLMWithTools to the LLMService interface |
| packages/core/src/services/base.llm.service.ts | Implemented two streaming methods using the AI SDK's streamText function, with support for multiple providers (OpenAI, Anthropic, OpenRouter) and tool execution tracking |
| packages/core/package.json | Version bump to 0.36.0 to reflect the new streaming functionality |
| .changeset/cold-dancers-repair.md | Added changeset documenting the minor version update with streaming response support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (sendUpdate) { | ||
| sendUpdate({ | ||
| type: "tool_response", | ||
| content: JSON.stringify(content), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Same inconsistency: sendUpdate is checked for existence but is defined as required in the type definition.
| tools = [], | ||
| temperature = 0.1, | ||
| maxOutputTokens, | ||
| traceContext, |
There was a problem hiding this comment.
The traceContext variable is destructured from options but never used in this function. Unlike runLLM which uses it for telemetry metadata (lines 241-242), this streaming function doesn't include it. Either remove the unused variable or add it to the metadata for consistency with runLLM.
| options: LLMServiceOptions & { tools?: any[] } | ||
| ): Promise<{ | ||
| textStream: AsyncIterable<string>; | ||
| generation: any; |
There was a problem hiding this comment.
The generation return type should be more specific. Consider using a proper type from the Langfuse SDK instead of any to improve type safety.
| maxOutputTokens, | ||
| }); | ||
|
|
||
| return { textStream: result.textStream, generation }; |
There was a problem hiding this comment.
The streaming methods don't complete the Langfuse generation tracking. Unlike runLLM which calls generation.end() with usage and cost data (lines 291-299), these streaming methods return the generation object without finalizing it. This could lead to incomplete telemetry data. Consider adding completion logic after the stream finishes, or documenting that the caller is responsible for ending the generation.
| if (sendUpdate) { | ||
| sendUpdate({ | ||
| type: "tool_execution", | ||
| content: JSON.stringify(content), | ||
| }); | ||
| } |
There was a problem hiding this comment.
The sendUpdate function is checked for existence before use (lines 435, 452), but according to LLMServiceOptions interface (llm.types.ts line 70), sendUpdate is a required field, not optional. Either make sendUpdate optional in the type definition by adding a ?, or remove these null checks since the field is guaranteed to exist.
| const runStreamedLLMWithTools = async ( | ||
| options: LLMServiceOptions & { tools?: any[] } | ||
| ): Promise<{ | ||
| textStream: AsyncIterable<string>; | ||
| generation: any; | ||
| }> => { |
There was a problem hiding this comment.
This streaming method also lacks documentation. Consider adding JSDoc comments similar to the suggestion for runStreamedLLM.
| messages, | ||
| temperature = 0.7, | ||
| maxOutputTokens, | ||
| traceContext, |
There was a problem hiding this comment.
The traceContext variable is destructured from options but never used in this function. Unlike runLLM which uses it for telemetry metadata (lines 241-242), this streaming function doesn't include it. Either remove the unused variable or add it to the metadata for consistency with runLLM.
| const runStreamedLLM = async (options: LLMServiceOptions): Promise<{ | ||
| textStream: AsyncIterable<string>; | ||
| generation: any; | ||
| }> => { |
There was a problem hiding this comment.
The new streaming methods lack documentation. Consider adding JSDoc comments to explain:
- When to use streaming vs. non-streaming methods
- The purpose and usage of the returned
generationobject - Whether the caller is responsible for calling
generation.end() - Example usage of consuming the
textStream
This would help API consumers understand how to properly use these methods.
| // Streaming methods (optional) | ||
| runStreamedLLM?(options: LLMServiceOptions): Promise<{ | ||
| textStream: AsyncIterable<string>; | ||
| generation: any; |
There was a problem hiding this comment.
The generation return type should be more specific. Consider using a proper type from the Langfuse SDK instead of any to improve type safety. This applies to both streaming methods.
| // Select AI model based on provider | ||
| let aiModel; | ||
| if (provider === "anthropic") { | ||
| aiModel = anthropic(model); | ||
| } else if (provider === "openrouter") { | ||
| const openrouter = createOpenRouter({ | ||
| apiKey: process.env.OPENROUTER_API_KEY, | ||
| }); | ||
| aiModel = openrouter.chat(model); | ||
| } else { | ||
| aiModel = openai(model); | ||
| } |
There was a problem hiding this comment.
The provider selection logic is duplicated. This is the third instance of the same code pattern. Consider extracting this into a shared helper function as suggested in the earlier comment.
No description provided.