-
Notifications
You must be signed in to change notification settings - Fork 17
feat: Add optional model configuration #38
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?
feat: Add optional model configuration #38
Conversation
WalkthroughThis pull request introduces configuration for an OpenAI-based AI assistant feature. Three new environment variables (_APP_ASSISTANT_BASE_URL, _APP_ASSISTANT_MODEL_NAME, and _APP_ASSISTANT_EMBEDDING_MODEL) are added to configuration files (.env.example, Dockerfile, docker-compose.yml) and type declarations (env.d.ts). The src/embeddings.js file is updated to consume these environment variables, configuring OpenAI embeddings with batch size, space settings, document validation, and model selection from environment defaults. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes follow a consistent, largely homogeneous pattern of adding the same three environment variables across configuration and type files, which reduces review complexity. However, the implementation changes in src/embeddings.js introduce multiple logic additions (baseURL configuration, model name selection, batch size, document validation, vector store space setting) that require verification for correctness and compatibility with the OpenAI client library, contributing moderate complexity overall. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
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: 1
🧹 Nitpick comments (2)
src/embeddings.js (1)
11-18: Good configurability improvements with minor refactor opportunity.The addition of configurable base URL, model name, and batch size enhances flexibility for OpenAI-compatible APIs. The implementation is correct.
Minor observations:
- The
baseURLfallback is duplicated at line 42 ingetOpenAIChat. Consider extracting to a constant if this pattern repeats elsewhere.- The
batchSize: 10is reasonable for rate limiting but is hardcoded. This could optionally be made configurable if different use cases require it..env.example (1)
2-4: Update embedding model to current version; consider alphabetical key ordering.Verification confirms
gpt-4ois a valid current chat completion model. However, thetext-embedding-ada-002model is deprecated in favor of newertext-embedding-3-largeandtext-embedding-3-smallmodels. For best practices, update the embedding model default to one of the v3 options. Additionally, the static analysis tool flags alphabetical key ordering, which improves maintainability of configuration files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.env.example(1 hunks)Dockerfile(1 hunks)docker-compose.yml(1 hunks)env.d.ts(1 hunks)src/embeddings.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/embeddings.js (1)
src/documents.js (2)
getDocuments(67-74)getDocuments(67-74)
🪛 dotenv-linter (4.0.0)
.env.example
[warning] 2-2: [UnorderedKey] The _APP_ASSISTANT_BASE_URL key should go before the _APP_ASSISTANT_OPENAI_API_KEY key
(UnorderedKey)
[warning] 3-3: [UnorderedKey] The _APP_ASSISTANT_MODEL_NAME key should go before the _APP_ASSISTANT_OPENAI_API_KEY key
(UnorderedKey)
[warning] 4-4: [UnorderedKey] The _APP_ASSISTANT_EMBEDDING_MODEL key should go before the _APP_ASSISTANT_MODEL_NAME key
(UnorderedKey)
🔇 Additional comments (5)
docker-compose.yml (1)
10-12: LGTM! Environment variables properly configured for pass-through.The three new environment variables follow the same pass-through pattern as the existing
_APP_ASSISTANT_OPENAI_API_KEY, allowing flexibility for different deployment environments.Dockerfile (1)
53-55: LGTM! Environment variables properly configured with sensible defaults.The default values are consistent with
.env.exampleand provide reasonable fallbacks for production deployments.env.d.ts (1)
5-7: LGTM! Type declarations are accurate and well-integrated.The optional string types correctly reflect the usage pattern in
src/embeddings.jswhere fallback values are provided via the||operator.src/embeddings.js (2)
26-28: LGTM! Cosine distance is appropriate for OpenAI embeddings.Explicitly specifying
space: "cosine"is good practice. Cosine similarity is the standard distance metric for normalized embeddings like those from OpenAI's text-embedding models.
37-58: LGTM! Chat model configuration properly implements environment-based selection.The configurable model name and base URL enable flexibility for different OpenAI-compatible APIs while maintaining sensible defaults.
| if (documents.length === 0) { | ||
| throw new Error("No documents found. Make sure to run 'pnpm run fetch-sources' first."); | ||
| } |
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.
Improve error message for production environments.
The document validation is excellent defensive programming. However, the error message references pnpm run fetch-sources, which assumes a development environment and may confuse users in production Docker deployments where the sources should already be present.
Consider a more environment-agnostic message:
- throw new Error("No documents found. Make sure to run 'pnpm run fetch-sources' first.");
+ throw new Error("No documents found. Ensure the sources directory is populated with documentation files.");📝 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.
| if (documents.length === 0) { | |
| throw new Error("No documents found. Make sure to run 'pnpm run fetch-sources' first."); | |
| } | |
| if (documents.length === 0) { | |
| throw new Error("No documents found. Ensure the sources directory is populated with documentation files."); | |
| } |
🤖 Prompt for AI Agents
In src/embeddings.js around lines 22 to 24, the thrown error references a
dev-only command ("pnpm run fetch-sources") which can confuse production users;
replace that hardcoded instruction with an environment-agnostic message that
explains no documents were found and instructs the operator to ensure source
data is present or that any ingestion step has been run (for dev, suggest
running the fetch step) and include brief guidance to check
deployment/configuration for where sources should be located.
What does this PR do?
Add a large model compatible with OpenAI protocol to the assistant
Test Plan
Related PRs and Issues
Have you read the Contributing Guidelines on issues?
OK
Summary by CodeRabbit