-
Notifications
You must be signed in to change notification settings - Fork 59.9k
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 custom models setting #6000
base: main
Are you sure you want to change the base?
Conversation
@QAbot-zh is attempting to deploy a commit to the NextChat Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces modifications to model validation and handling across multiple files. In Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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: 0
🧹 Nitpick comments (2)
app/utils/model.ts (1)
129-129
: Ensure consistent sorting logic when building custom model entries.Using
CustomSeq.next
with<modelName>@<providerType>
is a valid approach. However, consider adding a brief comment about why the sequence is based on both the model name and provider type, which could help future maintainers quickly grasp the rationale behind this design.Here’s a small diff you might consider:
+ // Use modelName@providerType to ensure uniqueness among custom providers sorted: CustomSeq.next(`${customModelName}@${provider?.providerType}`),
test/model-available.test.ts (1)
56-56
: Remove or address this FIXME to maintain a clean test suite.Leaving unfinished test cases may introduce confusion. If the test is genuinely problematic, consider deleting or refactoring it into a passing state.
Would you like me to propose a new test scenario that accurately covers multiple providers and ensures the correct pass/fail behavior?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/api/common.ts
(1 hunks)app/utils/model.ts
(2 hunks)test/model-available.test.ts
(2 hunks)
🧰 Additional context used
📓 Learnings (1)
test/model-available.test.ts (1)
Learnt from: code-october
PR: ChatGPTNextWeb/ChatGPT-Next-Web#5883
File: test/model-available.test.ts:46-52
Timestamp: 2024-11-30T07:30:14.257Z
Learning: 在测试文件中,对于没有指定 provider 的模型,`providerNames` 应该设置为 `modelName`,`"Default"` 不是有效的 provider 参数,应避免使用。
🔇 Additional comments (4)
app/api/common.ts (1)
127-127
: Consider verifying that "custom" as a provider does not overwrite a genuine provider.
By hardcoding the provider as "custom," there is a potential risk of overshadowing a valid provider if it wasn't properly identified earlier. Ensure that all genuine providers (e.g., "OpenAI", "Azure") are still routed correctly, and only truly unspecified models default to "custom."
Would you like a follow-up script that checks usage patterns of this provider in other files to confirm that “custom” is only used when no provider is determined?
app/utils/model.ts (1)
69-70
: Implementation aligns with the new naming convention.
You've updated the model name to use <modelName>@<providerType>
as the full identifier. This fits the PR objective of standardizing how custom providers are handled. Make sure all other references in the codebase have been updated accordingly.
test/model-available.test.ts (2)
72-79
: Great coverage for custom models.
This test ensures that a user-specified model without an explicit provider defaults to “custom.” The assertion expect(result).toBe(false);
correctly validates that the model is available.
81-84
: Verify handling of non-standard providers.
By specifying "custom"
for providerNames
, you confirm that models with any unusual suffix (e.g., @DeepSeek
) are recognized correctly. Ensure that additional exotic providers similarly fall under the correct logic if introduced in the future.
Your build has completed! |
这个 fail 看起来是另一个 vision-model-checker.test.ts 引起的,有什么办法只测试和当前 pr 相关的 test 文件吗 @Dogtiti |
💻 变更类型 | Change Type
🔀 变更说明 | Description of Change
在修复模型可用性检验后产生的新问题,在原项目中用户可以省略后缀
@OpenAI
或者使用任意非标准后缀标注@xxx
来通过 OpenAI 格式调用指定模型,任意后缀导致无法直接通过模型名称索引到 modelTable,将 fullname 修改为<modelName>@<providerType>
的形式,所有非标后缀使用默认 custom 作为后缀修饰,确保模型可以被索引到一些相关话题:
#5001 (comment)
https://linux.do/t/topic/318245
📝 补充信息 | Additional Information
Summary by CodeRabbit
Bug Fixes
Refactor
Tests