-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
add openai env variables #826
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
base: main
Are you sure you want to change the base?
Conversation
add custom base_url add custom models
Someone is attempting to deploy a commit to the Listinai Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes update OpenAI integration across several backend and library files. Environment variables are introduced for specifying OpenAI API endpoints and model names, replacing hardcoded values. The explicit use of the OpenAI API key is removed from code, and model selection is now driven by environment variables, with sensible defaults provided. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Backend API
participant OpenAI Service
Client->>Backend API: Request (e.g., chat, image generation)
Backend API->>OpenAI Service: Prepare OpenAI call (model/env vars)
OpenAI Service->>OpenAI: Call with model from env var (no explicit API key)
OpenAI-->>OpenAI Service: Response
OpenAI Service-->>Backend API: Result
Backend API-->>Client: Response
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/backend/src/api/routes/copilot.controller.tsOops! Something went wrong! :( ESLint: 8.57.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@eslint/eslintrc' imported from /eslint.config.mjs libraries/nestjs-libraries/src/database/prisma/autopost/autopost.service.tsOops! Something went wrong! :( ESLint: 8.57.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@eslint/eslintrc' imported from /eslint.config.mjs libraries/nestjs-libraries/src/openai/openai.service.tsOops! Something went wrong! :( ESLint: 8.57.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@eslint/eslintrc' imported from /eslint.config.mjs 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
remove OPENAI_API_KEY remove OPENAI_API_BASE_URL can be omitted, openai package already uses envs
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.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/backend/src/api/routes/copilot.controller.ts (2)
17-17
: Clarify the environment variable check logic and improve the comment.The logic
!process.env.OPENAI_API_KEY && !process.env.OPENAI_BASE_URL
will only abort if both variables are missing. While this supports the use case of custom servers without API keys, the comment "if using official OpenAI API, abort if no key" doesn't accurately describe this behavior.Consider updating the comment to better reflect the actual logic:
- !process.env.OPENAI_API_KEY && !process.env.OPENAI_BASE_URL // if using offical OpenAI API, abort if no key + !process.env.OPENAI_API_KEY && !process.env.OPENAI_BASE_URL // abort only if both API key and base URL are missing
26-34
: Consider adding environment variable validation.The implementation directly uses environment variables without validation. Invalid model names could cause runtime errors when making API calls.
Consider adding validation for the environment variables:
+ const textModel = process.env.OPENAI_TEXT_MODEL || 'gpt-4'; + const textModelMini = process.env.OPENAI_TEXT_MODEL_MINI || 'gpt-4o-mini'; + + // Basic validation could be added here + if (!textModel.startsWith('gpt-')) { + Logger.warn(`Potentially invalid OpenAI model name: ${textModel}`); + } const copilotRuntimeHandler = copilotRuntimeNestEndpoint({ endpoint: '/copilot/chat', runtime: new CopilotRuntime(), serviceAdapter: new OpenAIAdapter({ model: // @ts-ignore req?.body?.variables?.data?.metadata?.requestType === 'TextareaCompletion' - ? (process.env.OPENAI_TEXT_MODEL_MINI || 'gpt-4o-mini') - : (process.env.OPENAI_TEXT_MODEL || 'gpt-4.1'), + ? textModelMini + : textModel, }), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.env.example
(1 hunks)apps/backend/src/api/routes/copilot.controller.ts
(2 hunks)libraries/nestjs-libraries/src/agent/agent.graph.insert.service.ts
(1 hunks)libraries/nestjs-libraries/src/agent/agent.graph.service.ts
(1 hunks)libraries/nestjs-libraries/src/database/prisma/autopost/autopost.service.ts
(1 hunks)libraries/nestjs-libraries/src/openai/openai.service.ts
(8 hunks)
🧰 Additional context used
🪛 dotenv-linter (3.3.0)
.env.example
[warning] 77-77: [ValueWithoutQuotes] This value needs to be surrounded in quotes
[warning] 79-79: [ValueWithoutQuotes] This value needs to be surrounded in quotes
🔇 Additional comments (3)
libraries/nestjs-libraries/src/agent/agent.graph.insert.service.ts (1)
11-14
: LGTM! Good environment variable integration.The changes correctly remove explicit API key usage and implement environment-driven model selection with a sensible fallback. This supports the PR's objective of enabling custom OpenAI-compatible servers.
libraries/nestjs-libraries/src/openai/openai.service.ts (1)
7-7
: LGTM! Correct removal of explicit API key.The empty constructor allows the OpenAI client to use environment variables automatically, which aligns with the PR objectives.
apps/backend/src/api/routes/copilot.controller.ts (1)
31-32
: Verify the default model namegpt-4.1
.The default model name
'gpt-4.1'
appears to be non-standard. OpenAI typically uses model names likegpt-4
,gpt-4-turbo
,gpt-4o
, etc. This could lead to API errors if used with the official OpenAI API.Please verify that
gpt-4.1
is a valid model name:What are the current available OpenAI GPT-4 model names and is "gpt-4.1" a valid model?
If this is intended for a custom server, consider using a more standard default or adding validation to ensure the model name is appropriate for the configured endpoint.
const model = new ChatOpenAI({ | ||
apiKey: process.env.OPENAI_API_KEY || 'sk-proj-', | ||
model: 'gpt-4.1', | ||
model: process.env.OPENAI_TEXT_MODEL || 'gpt-4.1', | ||
temperature: 0.7, | ||
}); | ||
|
||
const dalle = new DallEAPIWrapper({ | ||
apiKey: process.env.OPENAI_API_KEY || 'sk-proj-', | ||
model: 'gpt-image-1', | ||
model: process.env.OPENAI_IMAGE_MODEL || 'gpt-image-1', | ||
}); |
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.
Fix invalid model names in both ChatOpenAI and DallEAPIWrapper.
Both fallback model names are invalid:
'gpt-4.1'
is not a valid OpenAI text model'gpt-image-1'
is not a valid OpenAI image model and inconsistent with other files that use'dall-e-3'
Apply this diff to use valid model names:
- model: process.env.OPENAI_TEXT_MODEL || 'gpt-4.1',
+ model: process.env.OPENAI_TEXT_MODEL || 'gpt-4',
- model: process.env.OPENAI_IMAGE_MODEL || 'gpt-image-1',
+ model: process.env.OPENAI_IMAGE_MODEL || 'dall-e-3',
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const model = new ChatOpenAI({ | |
apiKey: process.env.OPENAI_API_KEY || 'sk-proj-', | |
model: 'gpt-4.1', | |
model: process.env.OPENAI_TEXT_MODEL || 'gpt-4.1', | |
temperature: 0.7, | |
}); | |
const dalle = new DallEAPIWrapper({ | |
apiKey: process.env.OPENAI_API_KEY || 'sk-proj-', | |
model: 'gpt-image-1', | |
model: process.env.OPENAI_IMAGE_MODEL || 'gpt-image-1', | |
}); | |
const model = new ChatOpenAI({ | |
model: process.env.OPENAI_TEXT_MODEL || 'gpt-4', | |
temperature: 0.7, | |
}); | |
const dalle = new DallEAPIWrapper({ | |
model: process.env.OPENAI_IMAGE_MODEL || 'dall-e-3', | |
}); |
🤖 Prompt for AI Agents
In libraries/nestjs-libraries/src/database/prisma/autopost/autopost.service.ts
between lines 34 and 41, the fallback model names for ChatOpenAI and
DallEAPIWrapper are invalid. Replace 'gpt-4.1' with a valid OpenAI text model
name such as 'gpt-4', and replace 'gpt-image-1' with the consistent and valid
image model name 'dall-e-3' to align with other files.
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.
You forgot to use an API Key in the OpenAI API requests, correct me if I'm wrong please.
maintainer, i see there were some inconsistencies with some model names, was this intended? |
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.
Above
You pinged a random person right now, @nevo-david is the right person |
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.
Alright, I see that this is in fact true, but please reimplement it just in case something goes wrong
Any updates? |
Add ENV variables to change OpenAI model types.
Update example env file
What kind of change does this PR introduce?
This PR removes the
OPEN_API_KEY
variables from the openai constructor calls.The offical openai node package already uses ENVs for setting the
OPENAI_API_KEY
andOPENAI_BASE_URL
.So only the model name overrides were missing.
New Envs:
OPENAI_TEXT_MODEL
defaults togpt-4.1
OPENAI_TEXT_MODEL_MINI
defaults togpt-4o-mini
OPENAI_IMAGE_MODEL
defaults togpt-image-1
Why was this change needed?
See Issue #256
Also:
Some custom OpenAI compatible server require no API-Key, see commit e1486f7
Other information:
Tested with myLabs AI Server Platform (at university wide instance)
Example envs:
Checklist:
Put a "X" in the boxes below to indicate you have followed the checklist;
Summary by CodeRabbit