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.

Sign up for GitHub

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 defaultModel undefined error #5071

Merged
merged 5 commits into from
Jul 25, 2024
Merged

Fix defaultModel undefined error #5071

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2fd68bc
Fix defaultModel undefined error
ZTH7 Jul 21, 2024
aa4e855
Compatibility changes
ZTH7 Jul 22, 2024
46a47db
Merge branch 'ChatGPTNextWeb:main' into main
ZTH7 Jul 25, 2024
5c04d3c
Change method
ZTH7 Jul 25, 2024
c4334d4
Update model.ts
ZTH7 Jul 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions app/utils/model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,9 @@ export function collectModelTableWithDefaultModel(
defaultModel: string,
) {
let modelTable = collectModelTable(models, customModels);
if (defaultModel && defaultModel !== "") {
if (defaultModel && defaultModel !== "" && defaultModel in modelTable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please see L82:

modelTable[`${customModelName}@${provider?.id}`] = {...}

Copy link
Contributor Author

@ZTH7 ZTH7 Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Pardon me, I don't understand your meaning. I know modelTable's key include the provider name, but the defaultModel should also specify the provider. Is there any problem here?

Copy link
Contributor

Choose a reason for hiding this comment

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

我的意思是，当前系统已经是一个在线上运行了挺长时间，并且有很多人自己部署的系统。
所以，牵涉到配置相关的东西，需要尽量做到旧的配置能平稳迁移到新系统。
所以，这里可能需要做一下兼容（当用户旧的defaultModel没有配置provider name的时候，期望也能正常生效）

Copy link
Contributor

Choose a reason for hiding this comment

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

translate:

I mean, the current system is one that has been running online for a long time and has been deployed by many people themselves.
Therefore, when it comes to configuration-related things, it is necessary to ensure that the old configuration can be smoothly migrated to the new system.
Therefore, some compatibility may be required here (when the user's old defaultModel does not have the provider name configured, it is expected to take effect normally).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

了解了，但鉴于用户旧的default_model没有指定provider，我选择创建一个独立的可选项，而不是帮他自动选择一个provider（如图）。
这同时也兼容了用户旧的default_model所指定的模型可能不在custom_models中的情况。
image

Copy link
Contributor

Choose a reason for hiding this comment

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

或许针对就配置，帮用户选中匹配到的第一个才是合理的？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

但我们选到的第一个provider不一定可用，同时也会让这个函数变得繁琐，因为如果没有遍历到对应的模型，我们依旧需要create一个新element。
事实上我认为由用户来指定一个provider是必要的，如果用户没有指定，我们或许不应该随机选择。
当然如果你坚持，我将进行修改，毕竟两种方案在我看来都是可行的。

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. 确实，选到第一个不一定可用

但是，选到第一个可用的概率其实比较大

  1. 没有遍历到的时候，还是需要create新的element

这个逻辑，感觉不是很合理。应该是在available=true的模型中选择modelName=defaultModelName的第一个，标记一下isDefault=true
如果创建新的，感觉不合理，而且可能导致这个新创建的模型并不能正确的进行对话

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. 确实，选到第一个不一定可用

但是，选到第一个可用的概率其实比较大

  1. 没有遍历到的时候，还是需要create新的element

这个逻辑，感觉不是很合理。应该是在available=true的模型中选择modelName=defaultModelName的第一个，标记一下isDefault=true
如果创建新的，感觉不合理，而且可能导致这个新创建的模型并不能正确的进行对话

Copy link
Contributor Author

@ZTH7 ZTH7 Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

是的，我也感觉创建一个新的不合理，但由于在之前的版本并没有检查defaultModel是否在modelTable中，因此我担心有用户的配置是靠这个"bug"生效的。
如果你觉得可以，我也打算如果找不到便不再创建新的，以免这个新创建的模型并不能正确的进行对话。

modelTable[defaultModel] = {
...modelTable[defaultModel],
name: defaultModel,
available: true,
isDefault: true,
};
Expand Down
Loading