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

fix: add a method to detect vision model #4310

Conversation

wayoungofustc
Copy link

Title: Add environment variable $CUSTOM_VISION_MODELS for additional vision model discrimination

Description:
For user-defined models, there are some scenarios that do not conform to the design of the commit fix vision detect method.
For example: CUSTOM_MODELS=+claude-3-opus-20240229,+claude-3-sonnet-20240229

Considering this scenario, renaming the model by adding "vision" to its name is a feasible method.
However, this method imposes a cognitive burden on users, as memorizing and understanding the lengthened or changed model names are required.

Therefore, this commit introduces a method that adds an environment variable $CUSTOM_VISION_MODELS to provide additional discrimination for vision models.

It has been tested locally.
Test method:
export CUSTOM_MODELS=+claude-3-opus-20240229,+claude-3-sonnet-20240229,+gpt-4-1106-preview,+gpt-4-0125-preview,+gpt-4-32k
export CUSTOM_VISION_MODELS=+claude-3-opus-20240229,+claude-3-sonnet-20240229
yarn dev

Check the browser at http://localhost:3000/
When switching the model to claude-3-opus-20240229, the image upload button is displayed.
When switching to gpt-4-32k, the image upload button is not displayed.

Copy link

vercel bot commented Mar 15, 2024

@wayoungofustc is attempting to deploy a commit to the NextChat Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

Your build has completed!

Preview deployment

Copy link
Contributor

@H0llyW00dzZ H0llyW00dzZ left a comment

Choose a reason for hiding this comment

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

avoid to using || that complexity

app/utils.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@H0llyW00dzZ H0llyW00dzZ left a comment

Choose a reason for hiding this comment

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

now looks better plus easy maintain unlike complexity using ||

@wayoungofustc
Copy link
Author

now looks better plus easy maintain unlike complexity using ||

I apologize for my oversight in not properly handling the empty strings. Now maybe this pr is ready to be merged.

@H0llyW00dzZ
Copy link
Contributor

now looks better plus easy maintain unlike complexity using ||

I apologize for my oversight in not properly handling the empty strings. Now maybe this pr is ready to be merged.

It's fine because it's easier to maintain anyway, unlike when you were using the || operator and calling another model.includes, which added complexity and violated the Don't Repeat Yourself (DRY) principle.

@Algorithm5838
Copy link
Contributor

Algorithm5838 commented Mar 17, 2024

Thank you for the suggestion. However, I would prefer using a symbol, * for example, to denote multimodal models in environment variables, similar to the existing symbols for adding, removing, or renaming models. For example, +*claude-3-x. This approach, I assume, would be easier to implement and provide better control over the order of custom models.

@wayoungofustc
Copy link
Author

Thank you for the suggestion. However, I would prefer using a symbol, * for example, to denote multimodal models in environment variables, similar to the existing symbols for adding, removing, or renaming models. For example, +*claude-3-x. This approach, I assume, would be easier to implement and provide better control over the order of custom models.

Thank you for implementing the wildcard functionality. It's certainly a valuable feature to have. However, I believe it would also be beneficial to provide an option for exact matching without wildcards. This ensures precision when needed and caters to different use cases. What do you think about adding support for both wildcard and exact matching? Let me know your thoughts!

@Algorithm5838
Copy link
Contributor

I meant * to denote multimodal capabilities, not as a wildcard. So +claude-3-x refers to the text-only version, while +[*]claude-x refers to the multimodal version that can handle both text and vision inputs.
Also, I was just suggesting, I didn't implement a thing.

@wayoungofustc
Copy link
Author

@fred-bf any chance to merge this pr? thx

@pch18
Copy link

pch18 commented Mar 27, 2024

非常需要,期待合入

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


Very much needed, looking forward to incorporating

@wayoungofustc
Copy link
Author

@H0llyW00dzZ Hi Hollywoodzz, could you please help me ask the owner if this PR can be merged? Some API vendors do not support "claude-3-*" as the vision model.

@H0llyW00dzZ
Copy link
Contributor

@H0llyW00dzZ Hi Hollywoodzz, could you please help me ask the owner if this PR can be merged? Some API vendors do not support "claude-3-*" as the vision model.

@wayoungofustc
I am not the owner of this repository. Also, the reason I added claude-3 was for later use, so I don't have to write it again.

@wayoungofustc
Copy link
Author

@Dean-YZG
Hello,
Can you help me check if this pull request can be merged, whether I need to make any further modifications, or if this feature is not favored? Considering that some custom models require deployers to manually specify whether they support visualization features (as things often vary for each deployer and each model), we believe this is a valuable optional feature that will not introduce breaking changes.

@wayoungofustc
Copy link
Author

😞

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically.


😞

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.

None yet

5 participants