这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@timothycarambat
Copy link
Member

@timothycarambat timothycarambat commented Oct 28, 2023

resolves #157

Moves LLMs and Embedding into two different classes with inheritance where applicable
New Anthropic class to support calls made
Anthropic response parser using XML tags as suggested by docs.
Onboarding Support and Flow change depending on LLM selection
Sidebar item for embedding provider - empty if not needed
Remove LegacySettings folder
Simplify chat interface inside of vector stores

@review-agent-prime
Copy link

The PR seems to be adding support for a new language model provider, Anthropic, and also adding a new page for embedding preferences. Here are some suggestions:

  1. In frontend/src/components/PrivateRoute/index.jsx, you are adding a new key AnthropicApiKey to the system keys. Please ensure that the backend is updated to support this new key.

  2. In frontend/src/pages/GeneralSettings/LLMPreference/index.jsx, you are adding a new option for the LLM provider, Anthropic. Please ensure that the backend is updated to support this new provider.

  3. In frontend/src/pages/GeneralSettings/LLMPreference/index.jsx, you are adding a new input field for the Anthropic API key. Please ensure that the input is properly validated before being sent to the backend.

  4. In frontend/src/pages/GeneralSettings/LLMPreference/index.jsx, you are adding a new input field for the Anthropic Model Preference. Please ensure that the input is properly validated before being sent to the backend.

  5. In frontend/src/components/SettingsSidebar/index.jsx, you are adding a new option for Embedding Preference. Please ensure that the corresponding page is properly implemented.

  6. In frontend/src/pages/OnboardingFlow/OnboardingModal/index.jsx, you are adding a new step for Embedding Preference. Please ensure that the corresponding step is properly implemented.

  7. In frontend/src/App.jsx, you are adding a new route for Embedding Preference. Please ensure that the corresponding page is properly implemented.

  8. In frontend/src/components/Modals/LegacySettings/index.jsx and other files, you are removing a lot of code. Please ensure that these changes do not break any existing functionality.

  9. In frontend/src/pages/GeneralSettings/LLMPreference/index.jsx, you are adding a new condition for the LLM choice. Please ensure that this condition is properly handled in the backend.

  10. In frontend/src/pages/GeneralSettings/LLMPreference/index.jsx, you are adding a new condition for the LLM choice. Please ensure that this condition is properly handled in the backend.

  11. In frontend/src/pages/GeneralSettings/LLMPreference/index.jsx, you are adding a new condition for the LLM choice. Please ensure that this condition is properly handled in the backend.

  12. In frontend/src/pages/GeneralSettings/LLMPreference/index.jsx, you are adding a new condition for the LLM choice. Please ensure that this condition is properly handled in the backend.

  13. In frontend/src/pages/GeneralSettings/LLMPreference/index.jsx, you are adding a new condition for the LLM choice. Please ensure that this condition is properly handled in the backend.

  14. In frontend/src/pages/GeneralSettings/LLMPreference/index.jsx, you are adding a new condition for the LLM choice. Please ensure that this condition is properly handled in the backend.

  15. In frontend/src/pages/GeneralSettings/LLMPreference/index.jsx, you are adding a new condition for the LLM choice. Please ensure that this condition is properly handled in the backend.

  16. In frontend/src/pages/GeneralSettings/LLMPreference/index.jsx, you are adding a new condition for the LLM choice. Please ensure that this condition is properly handled in the backend.

  17. In frontend/src/pages/GeneralSettings/LLMPreference/index.jsx, you are adding a new condition for the LLM choice. Please ensure that this condition is properly handled in the backend.

  18. In frontend/src/pages/GeneralSettings/LLMPreference/index.jsx, you are adding a new condition for the LLM choice. Please ensure that this condition is properly handled in the backend.

  19. In frontend/src/pages/GeneralSettings/LLMPreference/index.jsx, you are adding a new condition for the LLM choice. Please ensure that this condition is properly handled in the backend.

  20. In frontend/src/pages/GeneralSettings/LLMPreference/index.jsx, you are adding a new condition for the LLM choice. Please ensure that this condition is properly handled in the backend.
    The PR seems to be adding new features and making changes to the frontend of the application. Here are some suggestions:

  21. In the EmbeddingSelection component, you are using alert to display error messages. It would be better to use a more user-friendly way to display errors, like a toast notification or a modal.

if (error) {
  // alert(`Failed to save LLM settings: ${error}`, "error");
  showToast(`Failed to save LLM settings: ${error}`, "error");
  return;
}
  1. In the LLMSelection component, you are using a switch statement to decide which step to go to next based on the data.LLMProvider value. It would be more maintainable to use a mapping object instead of a switch statement. This way, if you need to add more providers in the future, you just need to update the mapping object.
const stepMapping = {
  "anthropic": 7,
  "default": 2
};

goToStep(stepMapping[data.LLMProvider] || stepMapping.default);
  1. In the LLMSelection component, you are using a lot of repeated code for the different provider options. It would be better to create a separate component for the provider option and reuse it.
const ProviderOption = ({ name, value, link, description, image }) => (
  <LLMProviderOption
    name={name}
    value={value}
    link={link}
    description={description}
    checked={llmChoice === value}
    image={image}
    onClick={updateLLMChoice}
  />
);

// Usage
<ProviderOption
  name="OpenAI"
  value="openai"
  link="openai.com"
  description="The standard option for most non-commercial use. Provides both chat and embedding."
  image={OpenAiLogo}
/>
  1. In the LLMSelection component, you are using a lot of repeated code for the different input fields. It would be better to create a separate component for the input field and reuse it.
const InputField = ({ label, type, name, placeholder, defaultValue, required = true }) => (
  <div className="flex flex-col w-60">
    <label className="text-white text-sm font-semibold block mb-4">
      {label}
    </label>
    <input
      type={type}
      name={name}
      className="bg-zinc-900 text-white placeholder-white placeholder-opacity-60 text-sm rounded-lg focus:border-white block w-full p-2.5"
      placeholder={placeholder}
      defaultValue={defaultValue}
      required={required}
      autoComplete="off"
      spellCheck={false}
    />
  </div>
);

// Usage
<InputField
  label="API Key"
  type="password"
  name="OpenAiKey"
  placeholder="OpenAI API Key"
  defaultValue={settings?.OpenAiKey ? "*".repeat(20) : ""}
/>

The PR seems to be removing a large chunk of code related to API key management and adding new code for managing embedding preferences. Here are a few suggestions:

  1. In the new code, you are using setHasChanges(!!error ? true : false); to set the state of hasChanges. This can be simplified to setHasChanges(!!error); as !!error will already return a boolean value.

  2. In the handleSubmit function, you are creating a new object data and then populating it with form entries. This can be simplified by using the Object.fromEntries function. Replace the following code:

const data = {};
const form = new FormData(e.target);
for (var [key, value] of form.entries()) data[key] = value;

with:

const data = Object.fromEntries(new FormData(e.target));
  1. In the handleSubmit function, you are setting hasChanges to true if there is an error. This might not be the expected behavior as changes have not been saved if there is an error. Consider revising this logic.

  2. In the updateChoice function, you are setting hasChanges to true whenever the function is called. This might lead to unnecessary re-renders if the function is called but the selection hasn't actually changed. Consider checking if the selection is different from the current state before setting hasChanges to true.

  3. In the LLMProviderOption component, you are using an onClick event on a div element. This is not recommended for accessibility reasons. Consider using a button or a similar interactive element instead.

  4. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  5. In the LLMProviderOption component, you are using a peer class in your CSS, but it is not defined anywhere in your code. Make sure to define it or remove it if it's not needed.

  6. In the LLMProviderOption component, you are using an img element without an alt attribute. This is not recommended for accessibility reasons. Consider adding a descriptive alt attribute.

  7. In the LLMProviderOption component, you are using a readOnly attribute on a checkbox input. This is not recommended as it can be confusing for users. Consider using the disabled attribute instead if you want to prevent the user from interacting with the checkbox.

  8. In the LLMProviderOption component, you are using a formNoValidate attribute on a checkbox input. This attribute is not needed on individual inputs and should be used on the form element instead if you want to disable validation for the entire form.

  9. In the LLMProviderOption component, you are using a hidden attribute on a checkbox input. This is not recommended as it can be confusing for users. Consider using CSS to hide the checkbox visually but keep it accessible for screen readers.

  10. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  11. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  12. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  13. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  14. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  15. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  16. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  17. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  18. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  19. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  20. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  21. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  22. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  23. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  24. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  25. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  26. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  27. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  28. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  29. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  30. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  31. In the LLMProviderOption component, you are using a label element but it is not associated with any form control. This is not recommended for accessibility reasons. Consider associating the label with a form control or using a different element.

  32. In the `LLMProviderOption
    The code looks good overall, but there are a few areas where improvements can be made:

  33. Error handling: In the handleSubmit function, you're setting the error state to the error returned from System.updateSystem(data). However, you're not handling the case where the promise is rejected. It would be better to wrap the async call in a try-catch block to handle any exceptions.

const handleSubmit = async (e) => {
  e.preventDefault();
  setSaving(true);
  setError(null);
  const data = {};
  const form = new FormData(e.target);
  for (var [key, value] of form.entries()) data[key] = value;
  try {
    const { error } = await System.updateSystem(data);
    setError(error);
    setHasChanges(!!error);
  } catch (err) {
    setError(err.message);
    setHasChanges(true);
  } finally {
    setSaving(false);
  }
};
  1. Code duplication: There's a lot of repeated code for each vector database option. You could create a list of vector database options and map over it to render each option. This would make your code more DRY (Don't Repeat Yourself) and easier to maintain.
const vectorDBOptions = [
  {
    name: "Chroma",
    value: "chroma",
    link: "trychroma.com",
    description: "Open source vector database you can host yourself or on the cloud.",
    image: ChromaLogo,
  },
  // ...other options
];

// In render
{vectorDBOptions.map((option) => (
  <VectorDBOption
    key={option.value}
    checked={vectorDB === option.value}
    onClick={updateVectorChoice}
    {...option}
  />
))}
  1. Form validation: It seems like you're not validating the form inputs before submitting them. You should validate the inputs to ensure they're in the correct format and meet any other requirements your application has. You can use libraries like Formik or React Hook Form to help with this.

  2. State management: You're using a lot of useState hooks to manage the state of this component. Consider using useReducer instead, which is more suited to handling complex state logic that involves multiple sub-values. It also lets you optimize performance for components that trigger deep updates because you can pass dispatch down instead of callbacks.
    The PR looks good overall, but there are a few areas where improvements can be made:

  3. In server/utils/helpers/index.js, the getLLMProvider function has been modified to include a new case for "anthropic". However, the require statement for the Anthropic provider is missing. Please add the require statement for the Anthropic provider.

  4. In server/utils/chats/index.js, the chatWithWorkspace function has been modified to move the initialization of LLMConnector and VectorDb after the command check. This is a good change as it avoids unnecessary initialization when a command is present. However, please ensure that this change does not affect other parts of the code where these variables are used.

  5. In server/models/systemSettings.js, new environment variables have been added for the Anthropic provider. Please ensure that these environment variables are properly set in the deployment environment.

  6. In server/utils/vectorDbProviders/lance/index.js, server/utils/vectorDbProviders/chroma/index.js, and other similar files, the constructPrompt function has been added to the LLMConnector. This is a good change as it abstracts the prompt construction logic. However, please ensure that this function is properly implemented in all LLM providers.

  7. In server/utils/EmbeddingEngines/openAi/index.js and server/utils/EmbeddingEngines/azureOpenAi/index.js, new classes OpenAiEmbedder and AzureOpenAiEmbedder have been added. Please ensure that these classes are properly tested.

  8. In server/utils/AiProviders/openAi/index.js and server/utils/AiProviders/azureOpenAi/index.js, the classes OpenAi and AzureOpenAi have been renamed to OpenAiLLM and AzureOpenAiLLM respectively, and they now extend the corresponding Embedder classes. This is a good change as it separates the LLM and embedding functionalities. However, please ensure that this change does not affect other parts of the code where these classes are used.

  9. In server/utils/helpers/updateENV.js, new validation functions have been added for the Anthropic provider. Please ensure that these functions are properly tested.
    The code looks good overall, but there are a few areas where improvements can be made:

  10. In the AnthropicLLM class, the AnthropicAI object is created in the constructor. This could potentially lead to issues if the AnthropicAI object needs to be re-initialized or if multiple instances of AnthropicLLM are created. Consider moving the creation of the AnthropicAI object to a separate method that can be called when needed.

class AnthropicLLM {
  constructor(embedder = null) {
    if (!process.env.ANTHROPIC_API_KEY)
      throw new Error("No Anthropic API key was set.");

    this.anthropic = this.initializeAnthropicAI();

    // rest of the code...
  }

  initializeAnthropicAI() {
    const AnthropicAI = require("@anthropic-ai/sdk");
    return new AnthropicAI({
      apiKey: process.env.ANTHROPIC_API_KEY,
    });
  }

  // rest of the code...
}
  1. The isValidChatModel method currently has a hardcoded list of valid models. If the list of valid models changes or grows, this method will need to be updated. Consider moving the list of valid models to a configuration file or environment variable so it can be easily updated without changing the code.
isValidChatModel(modelName = "") {
  const validModels = process.env.VALID_MODELS.split(",");
  return validModels.includes(modelName);
}
  1. The sendChat and getChatCompletion methods have a lot of duplicated code. Consider creating a helper method that both methods can use to reduce code duplication.
async createCompletion(prompt) {
  const model = process.env.ANTHROPIC_MODEL_PREF || "claude-2";
  if (!this.isValidChatModel(model))
    throw new Error(
      `Anthropic chat: ${model} is not valid for chat completion!`
    );

  return await this.anthropic.completions
    .create({
      model: "claude-2",
      max_tokens_to_sample: 300,
      prompt,
    })
    .then((res) => {
      const { completion } = res;
      const re =
        /(?:<anythingllmresponse>)([\s\S]*)(?:<\/anythingllmresponse>)/;
      const response = completion.match(re)?.[1]?.trim();
      if (!response)
        throw new Error("Anthropic: No response could be parsed.");
      return { content: response, error: null };
    })
    .catch((e) => {
      return { content: null, error: e.message };
    });
}

async sendChat(chatHistory = [], prompt, workspace = {}) {
  const { content, error } = await this.createCompletion(this.constructPrompt({
    systemPrompt: chatPrompt(workspace),
    userPrompt: prompt,
    chatHistory,
  }));

  if (error) throw new Error(error);
  return content;
}

async getChatCompletion(prompt = "", _opts = {}) {
  const { content, error } = await this.createCompletion(prompt);

  if (error) throw new Error(error);
  return content;
}
  1. The getEmbeddingEngineSelection function currently requires different modules based on the EMBEDDING_ENGINE environment variable. This could potentially lead to issues if a new embedding engine is added or if the name of an existing engine is changed. Consider using a dynamic require statement to load the appropriate module based on the environment variable.
function getEmbeddingEngineSelection() {
  const engineSelection = process.env.EMBEDDING_ENGINE;
  const Embedder = require(`../../EmbeddingEngines/${engineSelection}`);
  return new Embedder();
}

The PR seems to be adding support for a new language model provider, 'anthropic', in the environment configuration. This is a good addition, but there are a few things that could be improved:

  1. It's important to ensure that sensitive data like API keys are not hardcoded in the code or configuration files. Instead, they should be stored securely and accessed through secure means. For the ANTHROPIC_API_KEY and OPEN_AI_KEY, consider using a secure method to store and retrieve these keys.

  2. The comment # Using OpenAI as your embedding provider seems to be misplaced. If the intention is to allow the user to choose between different embedding providers, it would be better to structure the configuration in a way that makes this clear.

Here's a suggestion on how you could improve the code:

 ######## LLM API SElECTION ################
-# LLM_PROVIDER='openai'
+# Uncomment the provider you want to use
+# LLM_PROVIDER='openai'
 # OPEN_AI_KEY=
-# OPEN_MODEL_PREF='gpt-3.5-turbo'
+# OPEN_MODEL_PREF='gpt-3.5-turbo'
 
 # LLM_PROVIDER='azure'
 # AZURE_OPENAI_ENDPOINT=
 # AZURE_OPENAI_KEY=
 # OPEN_MODEL_PREF='my-gpt35-deployment' # This is the "deployment" on Azure you want to use. Not the base model.
 # EMBEDDING_MODEL_PREF='embedder-model' # This is the "deployment" on Azure you want to use for embeddings. Not the base model.
+# LLM_PROVIDER='anthropic'
+# ANTHROPIC_API_KEY= # Add your anthropic API key here
+# ANTHROPIC_MODEL_PREF='claude-2'
 
 ######## Embedding Provider Selection ########
+# Uncomment the provider you want to use for embeddings
+# EMBEDDING_ENGINE='openai'
+# OPEN_AI_KEY= # Add your OpenAI key here

This way, it's clear to the user that they need to uncomment the provider they want to use and where to add their API keys.
The PR looks good overall, but there are a couple of minor improvements that could be made:

  1. It's a good practice to lock the versions of your dependencies to avoid potential issues with future updates. You can do this by removing the caret (^) before the version number. This applies to the newly added dependency "@anthropic-ai/sdk".

  2. It's also a good practice to add a newline at the end of the file. This is a POSIX standard and it helps with file concatenation and other operations.

Here are the suggested changes:

  "dependencies": {
-    "@anthropic-ai/sdk": "^0.8.1",
+    "@anthropic-ai/sdk": "0.8.1",
     "@azure/openai": "^1.0.0-beta.3",
     "@googleapis/youtube": "^9.0.0",
     "@pinecone-database/pinecone": "^0.1.6",
    "@prisma/client": "5.3.0",
    "@qdrant/js-client-rest": "^1.4.0",
    "archiver": "^5.3.1",
    "bcrypt": "^5.1.0",
    "body-parser": "^1.20.2",

    "uuid-apikey": "^1.5.3",
    "vectordb": "0.1.12",
    "weaviate-ts-client": "^1.4.0"
  },
  "devDependencies": {
@@ -59,4 +60,4 @@
     "nodemon": "^2.0.22",
     "prettier": "^2.4.1"
   }
-}
\ No newline at end of file
+}
+

move embedding selector to general util
@timothycarambat timothycarambat merged commit 5d56ab6 into master Oct 30, 2023
@timothycarambat timothycarambat deleted the anthropic-claude-2-support branch October 30, 2023 22:44
franzbischoff referenced this pull request in franzbischoff/anything-llm Nov 4, 2023
Anthropic claude 2 support (#305)

* WIP Anythropic support for chat, chat and query w/context

* Add onboarding support for Anthropic

* cleanup

* fix Anthropic answer parsing
move embedding selector to general util
cabwds pushed a commit to cabwds/anything-llm that referenced this pull request Jul 3, 2025
* WIP Anythropic support for chat, chat and query w/context

* Add onboarding support for Anthropic

* cleanup

* fix Anthropic answer parsing
move embedding selector to general util
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.

Anthropic Claude 2 API Integration

2 participants