Skip to content
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

Resolve model connection info and add embeddings model candidates #624

Merged
merged 3 commits into from
Aug 16, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Aug 16, 2024

This pull request includes changes to resolve the model connection info, allowing for the selection of different models. It also adds a list of default embeddings model candidates to choose from. These changes improve the flexibility and customization options for the software.

  • A possibility for clients to be undefined in startServer function in server.ts is now handled by using optional chaining.

    • Before: if (!wss.clients.size) throw new Error("no llm clients connected")
    • After: if (!wss.clients?.size) throw new Error("no llm clients connected")
  • The list of DEFAULT_MODEL_CANDIDATES is expanded with "openai:gpt-4o" in constants.ts.

  • New array DEFAULT_EMBEDDINGS_MODEL_CANDIDATES is created in constants.ts. The array consists of four models named after tech companies and suffixed with "text-embedding-3-small".

  • Modifications in resolveModelConnectionInfo function in models.ts:

    • The function parameters are updated. Besides model and token, candidates is also added in options.
    • The candidates parameter is initialized with a default value that contains the default model and a spread operator for DEFAULT_MODEL_CANDIDATES.
    • The for loop that iterates over candidates is modified to use the candidates parameter.
  • Changes in vectorSearch function in vectorSearch.ts:

    • The function calls resolveModelConnectionInfo with additional parameters, where token value is true and candidates is a set of default settings for embedding model options.
  • Addition of a new file vectorstore.genai.mts in the sample directory. This file contains functions for initializing, creating a document, and searching in a vector store.

  • The sample package's package.json file is updated with a new devDependency vectorstore.

generated by pr-describe

@@ -98,7 +98,7 @@ export async function startServer(options: { port: string }) {
): Promise<ChatCompletionResponse> => {
const { messages, model } = req
const { partialCb } = options
if (!wss.clients.size) throw new Error("no llm clients connected")
if (!wss.clients?.size) throw new Error("no llm clients connected")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional chaining is used on wss.clients?.size. If wss.clients is undefined, it will not throw an error and may lead to unexpected behavior. Please ensure wss.clients is always defined.

generated by pr-review-commit optional_chaining

...DEFAULT_MODEL_CANDIDATES,
])
for (const candidate of candidates) {
for (const candidate of new Set(candidates || [])) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a new Set from candidates array is unnecessary if the array does not contain duplicate values. This can lead to unnecessary memory usage and performance degradation.

generated by pr-review-commit unnecessary_set

...DEFAULT_EMBEDDINGS_MODEL_CANDIDATES,
],
}
)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The object passed to resolveModelConnectionInfo function is complex and hard to read. Consider creating this object separately and then pass it to the function for better readability and maintainability.

generated by pr-review-commit complex_object_creation

Copy link

The changes in this pull request can be summarized as follows:

  1. 🛠️ Optional chaining has been added to the wss.clients in server.ts to handle the case when client is null or undefined. This is a good safety addition that prevents runtime errors.

  2. 📚 Constants update: New models have been added to DEFAULT_MODEL_CANDIDATES and a new constant DEFAULT_EMBEDDINGS_MODEL_CANDIDATES has been introduced in constants.ts. This expands the options for models to choose from and allows a greater flexibility.

  3. 💡 Model resolution changes: In models.ts, the resolveModelConnectionInfo function has been updated to accept an additional candidates option. This allows users to specify a custom list of candidate models. If no list is provided, it defaults to the DEFAULT_MODEL_CANDIDATES. This is a great customization feature.

  4. 🔎 Embedding models in vector search: In vectorsearch.ts, the vectorSearch function now also accepts a list of candidates for its embeddingsModel parameter. By default, it uses DEFAULT_EMBEDDINGS_MODEL_CANDIDATES. This results in an expanded functionality of the vector search.

Overall, the changes provide better error handling, more customization options and extended functionality. The code seems well-structured and no functional issues are observed.

However, in vectorsearch.ts, the candidates array could be abstracted to a constant for better code readability, but this is a minor issue and does not affect the functionality.

With these observations, I can say that overall the changes look good to me. So, it's LGTM 🚀.

generated by pr-review

@pelikhan pelikhan merged commit e6c207b into main Aug 16, 2024
10 checks passed
@pelikhan pelikhan deleted the vectorstore branch August 16, 2024 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant