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

feat: runtime configuration of vision-capable models #5919

Merged
merged 7 commits into from
Dec 22, 2024
16 changes: 16 additions & 0 deletions app/constant.ts
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,22 @@ export const DEFAULT_TTS_VOICES = [
"shimmer",
];

export const VISION_MODEL_REGEXES = [
/vision/,
/gpt-4o/,
/claude-3/,
/gemini-1\.5/,
/gemini-exp/,
/gemini-2\.0/,
/learnlm/,
/qwen-vl/,
/qwen2-vl/,
/gpt-4-turbo(?!.*preview)/, // Matches "gpt-4-turbo" but not "gpt-4-turbo-preview"
/^dall-e-3$/, // Matches exactly "dall-e-3"
];

export const EXCLUDE_VISION_MODEL_REGEXES = [/claude-3-5-haiku-20241022/];

const openaiModels = [
"gpt-3.5-turbo",
"gpt-3.5-turbo-1106",
Expand Down
30 changes: 9 additions & 21 deletions app/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { RequestMessage } from "./client/api";
import { ServiceProvider } from "./constant";
// import { fetch as tauriFetch, ResponseType } from "@tauri-apps/api/http";
import { fetch as tauriStreamFetch } from "./utils/stream";
import { VISION_MODEL_REGEXES, EXCLUDE_VISION_MODEL_REGEXES } from "./constant";

export function trimTopic(topic: string) {
// Fix an issue where double quotes still show in the Indonesian language
Expand Down Expand Up @@ -252,28 +253,15 @@ export function getMessageImages(message: RequestMessage): string[] {
}

export function isVisionModel(model: string) {
// Note: This is a better way using the TypeScript feature instead of `&&` or `||` (ts v5.5.0-dev.20240314 I've been using)

const excludeKeywords = ["claude-3-5-haiku-20241022"];
const visionKeywords = [
"vision",
"gpt-4o",
"claude-3",
"gemini-1.5",
"gemini-exp",
"gemini-2.0",
"learnlm",
"qwen-vl",
"qwen2-vl",
];
const isGpt4Turbo =
model.includes("gpt-4-turbo") && !model.includes("preview");

const envVisionModels = process.env.NEXT_PUBLIC_VISION_MODELS?.split(",").map(
Yiming3 marked this conversation as resolved.
Show resolved Hide resolved
(m) => m.trim(),
);
if (envVisionModels?.includes(model)) {
return true;
}
Dogtiti marked this conversation as resolved.
Show resolved Hide resolved
return (
!excludeKeywords.some((keyword) => model.includes(keyword)) &&
(visionKeywords.some((keyword) => model.includes(keyword)) ||
isGpt4Turbo ||
isDalle3(model))
!EXCLUDE_VISION_MODEL_REGEXES.some((regex) => regex.test(model)) &&
VISION_MODEL_REGEXES.some((regex) => regex.test(model))
);
}

Expand Down
67 changes: 67 additions & 0 deletions test/vision-model-checker.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import { isVisionModel } from "../app/utils";

describe("isVisionModel", () => {
const originalEnv = process.env;

beforeEach(() => {
jest.resetModules();
process.env = { ...originalEnv };
});

afterEach(() => {
process.env = originalEnv;
});

test("should identify vision models using regex patterns", () => {
const visionModels = [
"gpt-4-vision",
"claude-3-opus",
"gemini-1.5-pro",
"gemini-2.0",
"gemini-exp-vision",
"learnlm-vision",
"qwen-vl-max",
"qwen2-vl-max",
"gpt-4-turbo",
"dall-e-3",
];

visionModels.forEach((model) => {
expect(isVisionModel(model)).toBe(true);
});
});

test("should exclude specific models", () => {
expect(isVisionModel("claude-3-5-haiku-20241022")).toBe(false);
});
Comment on lines +34 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

πŸ› οΈ Refactor suggestion

Enhance exclusion testing with more patterns and documentation.

The current test is too specific. Consider:

  1. Adding more exclusion patterns
  2. Documenting why certain models are excluded
  3. Using test.each for better readability
-test("should exclude specific models", () => {
-  expect(isVisionModel("claude-3-5-haiku-20241022")).toBe(false);
-});
+test.each([
+  // Models excluded due to specific version constraints
+  ["claude-3-5-haiku-20241022", "Haiku variant without vision capabilities"],
+  // Add more excluded models with reasons
+])("should exclude %s (%s)", (model, reason) => {
+  expect(isVisionModel(model)).toBe(false);
+});

Committable suggestion skipped: line range outside the PR's diff.


test("should not identify non-vision models", () => {
const nonVisionModels = [
"gpt-3.5-turbo",
"gpt-4-turbo-preview",
"claude-2",
"regular-model",
];

nonVisionModels.forEach((model) => {
expect(isVisionModel(model)).toBe(false);
});
});

test("should identify models from NEXT_PUBLIC_VISION_MODELS env var", () => {
process.env.NEXT_PUBLIC_VISION_MODELS = "custom-vision-model,another-vision-model";

expect(isVisionModel("custom-vision-model")).toBe(true);
expect(isVisionModel("another-vision-model")).toBe(true);
expect(isVisionModel("unrelated-model")).toBe(false);
});

test("should handle empty or missing NEXT_PUBLIC_VISION_MODELS", () => {
process.env.NEXT_PUBLIC_VISION_MODELS = "";
expect(isVisionModel("unrelated-model")).toBe(false);

delete process.env.NEXT_PUBLIC_VISION_MODELS;
expect(isVisionModel("unrelated-model")).toBe(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

πŸ› οΈ Refactor suggestion

Replace delete operator with undefined assignment.

The delete operator can impact performance as flagged by static analysis.

-    delete process.env.NEXT_PUBLIC_VISION_MODELS;
-    expect(isVisionModel("unrelated-model")).toBe(false);
+    process.env.NEXT_PUBLIC_VISION_MODELS = undefined;
+    expect(isVisionModel("unrelated-model")).toBe(false);
πŸ“ 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.

Suggested change
delete process.env.NEXT_PUBLIC_VISION_MODELS;
expect(isVisionModel("unrelated-model")).toBe(false);
process.env.NEXT_PUBLIC_VISION_MODELS = undefined;
expect(isVisionModel("unrelated-model")).toBe(false);
🧰 Tools
πŸͺ› Biome (1.9.4)

[error] 63-63: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

expect(isVisionModel("gpt-4-vision")).toBe(true);
});
});