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

refactor: improve Defaults.Key #599

Merged
merged 23 commits into from
Jul 8, 2024
Merged

refactor: improve Defaults.Key #599

merged 23 commits into from
Jul 8, 2024

Conversation

tisfeng
Copy link
Owner

@tisfeng tisfeng commented Jul 4, 2024

Close #497

This PR does not include functional changes, it reduces a large amount of duplicate code related to LLMStreamService configuration, the main content is as follows:

  • Remove all static Defaults.Keys of LLMStreamService, replacing them with dynamic keys related to serviceType.
  • Use the instance method configurationListItems() to generate the configuration view.

This PR has many changes, hoping it doesn't have any breaking changes, please review carefully.

Jerry23011
Jerry23011 previously approved these changes Jul 5, 2024
AkaShark
AkaShark previously approved these changes Jul 5, 2024
@phlpsong
Copy link
Collaborator

phlpsong commented Jul 6, 2024

It looks like the Use Model config is not working as expected now:

Screenshot 2024-07-06 at 18 25 43

@tisfeng
Copy link
Owner Author

tisfeng commented Jul 6, 2024

It looks like the Use Model config is not working as expected now:

Screenshot 2024-07-06 at 18 25 43

How do you trigger this issue? Can you reproduce it?

@phlpsong
Copy link
Collaborator

phlpsong commented Jul 6, 2024

Yes, it seems pretty easy to reproduce. Some behavior occurs on my another device too.

Defaults(validModelsKey) always return empty string, it not get updated after supported models change.

Screenshot 2024-07-06 at 22 16 37

I just take a look at some code, I noticed that setupSubscribers has been called in EZBaseQueryViewController but not been invoked from new config side, maybe this is the root cause of this issue?

@tisfeng tisfeng dismissed stale reviews from AkaShark and Jerry23011 via b2485c3 July 6, 2024 14:42
@tisfeng
Copy link
Owner Author

tisfeng commented Jul 6, 2024

ok, I fixed it b2485c3 , please try it again.

@phlpsong
Copy link
Collaborator

phlpsong commented Jul 6, 2024

ok, I fixed it b2485c3 , please try it again.

The issue in BuiltInService has been fixed, but OpenAI and CustomAI service has the similar issue, Use Model not changed after input supported models.

Screenshot 2024-07-06 at 22 50 12

@tisfeng
Copy link
Owner Author

tisfeng commented Jul 7, 2024

ok, I will take a look later 🥲

@tisfeng
Copy link
Owner Author

tisfeng commented Jul 7, 2024

I tried to reset OpenAI modelKey, validModelsKey and supportedModelsKey to default value, then print validModels seems good.

I tested on the service configuration page, and Use Model follows the change of Supported Models, everything works fine.

I've checked the code and didn't find any issues, please test carefully or provide more problem information.

image

@phlpsong
Copy link
Collaborator

phlpsong commented Jul 7, 2024

I tried to reset these keys, but after input new values in supported models, the same issue still exists.

Screen.Recording.2024-07-07.at.18.17.11.mov

@tisfeng
Copy link
Owner Author

tisfeng commented Jul 7, 2024

After checking, I found that if I turn off OpenAI service, modifying the Supported Models at this time will not cause the Use Model to change, which is because we currently only monitor the services turned on in the query window.

This is a bug, and I will fix it later.

@phlpsong Is this the situation you encountered?

iShot_2024-07-07_21.25.36.mp4

@phlpsong
Copy link
Collaborator

phlpsong commented Jul 7, 2024

After checking, I found that if I turn off OpenAI service, modifying the Supported Models at this time will not cause the Use Model to change, which is because we currently only monitor the services turned on in the query window.

This is a bug, and I will fix it later.

@phlpsong Is this the situation you encountered?

iShot_2024-07-07_21.25.36.mp4

Not exactly the same, I tried to enable and disable the service, the Use Model still not get updated.

Screen.Recording.2024-07-07.at.22.13.12.mov

I also tried to reset these models first and give a try:

Screen.Recording.2024-07-07.at.22.15.37.mov

@tisfeng
Copy link
Owner Author

tisfeng commented Jul 7, 2024

No, what I mean is, you must first display a query window, and then turn on OpenAI service, only then will it start listening for changes in Defaults.

@phlpsong
Copy link
Collaborator

phlpsong commented Jul 8, 2024

No, what I mean is, you must first display a query window, and then turn on OpenAI service, only then will it start listening for changes in Defaults.

OK, it's working.

Screenshot 2024-07-08 at 08 44 56

@tisfeng
Copy link
Owner Author

tisfeng commented Jul 8, 2024

This bug is not easy to resolve, let's merge this PR first, and I'll fix it in another PR later.

@phlpsong
Copy link
Collaborator

phlpsong commented Jul 8, 2024

This bug is not easy to resolve, let's merge this PR first, and I'll fix it in another PR later.

ok, np

@tisfeng tisfeng merged commit 6d4cf39 into dev Jul 8, 2024
5 checks passed
@tisfeng tisfeng deleted the refactor-defaults branch July 8, 2024 06:03
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.

4 participants