Conversation
There was a problem hiding this comment.
PR Summary
Added multi-provider support to NeuralInk, expanding beyond OpenAI to include Anthropic and Cohere LLMs for article generation.
- Unused
langchaindependency inpackage.jsonshould be removed src/llms/providers.jsneeds error handling in generate functions and consistent system prompts across providers- Cohere client initialization in
src/llms/providers.jsshould be moved outside the generate function to avoid repeated initialization - API keys should be validated during provider client initialization
- Temperature parameter should be standardized across all providers in
src/llms/providers.js
3 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
| const cors = require('cors') | ||
| const OpenAI = require('openai') | ||
| const path = require('path') | ||
| const providers = require('./src/llm/providers') |
There was a problem hiding this comment.
logic: Path './src/llm/providers' doesn't match actual file location './src/llms/providers'
| client: cohere, | ||
| models: ['command', 'command-light'], | ||
| generate: async (client, model, prompt) => { | ||
| client.init(process.env.COHERE_API_KEY) |
There was a problem hiding this comment.
logic: Initializing Cohere client on every request is inefficient. Move client.init() to where other clients are initialized.
| const completion = await client.messages.create({ | ||
| model, | ||
| max_tokens: 500, | ||
| messages: [ | ||
| { | ||
| role: "user", | ||
| content: prompt | ||
| } | ||
| ] | ||
| }) |
There was a problem hiding this comment.
style: Missing system prompt and temperature settings that other providers use. Could lead to inconsistent article generation.
|
@greptile-apps does this PR have apis for adding/removing providers? |
There was a problem hiding this comment.
No, this PR doesn't include APIs for adding or removing providers dynamically. The PR only adds a GET endpoint /api/providers that lists the available providers and their models, but the providers themselves are statically defined in the providers.js file. To add or remove providers, you would need to modify the code directly in the providers.js file.
The changes mainly focus on:
- Adding support for multiple LLM providers (OpenAI, Anthropic, Cohere)
- Exposing an API to list available providers and their models
- Updating the generate-article endpoint to work with different providers
Would you like me to suggest how to add APIs for managing providers dynamically?
|
@greptile should we add apis for adding/removing providers? |
There was a problem hiding this comment.
I would not recommend adding APIs for adding/removing providers dynamically. Here's why:
-
Security: Allowing dynamic provider addition could introduce security vulnerabilities since providers have direct access to API keys and execute code.
-
Integration Complexity: Each provider requires specific client initialization, API key configuration, and unique generation logic. This would be difficult to handle safely through an API.
-
Code Quality: The current structure with providers defined in code ensures proper type checking, validation, and consistent implementation of the provider interface.
Instead, I would suggest:
- Keep providers configured through code
- Use feature flags or environment variables to enable/disable providers
- Follow proper code review and deployment processes for adding new providers
This gives you better control over the integration quality and security of the application. If you need flexibility, consider implementing provider configuration through environment variables or configuration files that are managed through your deployment process.
|
@henry-richard what do you think? |
|
@wanda-carlson lets ask coderabbit? |
|
Let me add it. |
|
@coderabbitai what do you think about greptile's suggestion about APIs? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3034 🏁 Script executed: Length of output: 7034 @wanda-carlson Based on my analysis of the codebase, I think adding APIs for adding/removing providers is an excellent suggestion that aligns perfectly with the PR's goal of giving users more flexibility with LLMs. Currently, the codebase has:
Adding APIs for provider management would offer several benefits:
I recommend implementing:
This would require:
Would you like more detailed suggestions on the implementation approach? |
Users need to be able to select their own LLMs and not be tied down to open ai!