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: improve setting ui in UI.wl, add OpenAi Chat Completion URL settings #456

Merged
merged 4 commits into from
Nov 20, 2023

Conversation

nhannht
Copy link
Contributor

@nhannht nhannht commented Nov 15, 2023

Now you can change the default endpoint to anything you want. 

Hello, this repo is pretty big, I try not to mess with the current code base but if I do something wrong, feel free to fix it if you have time

There are typical cases in which the user didn't fetch data from the official OpenAi url but from some kind of proxy, that is why I added this minor change.

image

By default, the official URL will be used, but now people can freely change it to something else like http://localhost:8080/v1/chat/completions/.

Now you can change default endpoint to anything you want
@bobohilario
Copy link
Collaborator

This seems like a good idea. Two small changes to make for consistency:

  • Do not change the gitignore file
  • Please use "OpenAI" with a capital I instead of "OpenAi" and "API" all capital instead of "Api" everywhere.

@nhannht
Copy link
Contributor Author

nhannht commented Nov 15, 2023

Sorry, I was confused when I looked at the CI checking, did the engine in the docker image have a problem with the license?

image

This is the screenshot from when I ran it on my local machine
image

image

image

@rhennigan
Copy link
Member

Sorry, I was confused when I looked at the CI checking, did the engine in the docker image have a problem with the license?

Don't worry about that, it was my mistake. I approved the CI/CD actions for your PR, but I wasn't considering the fact that it sourced from your forked repository instead of the WolframResearch/Chatbook repository. Your forked repository just isn't configured with the necessary GitHub secrets to run those workflows, hence the errors. You can safely ignore those.

Copy link
Member

@rhennigan rhennigan left a comment

Choose a reason for hiding this comment

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

This looks great, though there are a few changes we'd like before merging (see comments).

Also, you may have noticed in the code that there are two paths for submitting the chat request, depending on whether LLMServices is available (here and here). In 14.0+, the LLMServices method will be the way that requests are submitted, so your changes wouldn't have any affect in future versions. However, I think this is still definitely a useful feature for 13.3 users.

Source/Chatbook/UI.wl Outdated Show resolved Hide resolved
Source/Chatbook/UI.wl Outdated Show resolved Hide resolved
Source/Chatbook/SendChat.wl Outdated Show resolved Hide resolved
nhannht and others added 3 commits November 16, 2023 09:42
![base on this comment](WolframResearch#456 (comment)) and ![these comments](WolframResearch#456 (review))

```txt
This seems like a good idea. Two small changes to make for consistency:

 Do not change the gitignore file
 Please use "OpenAI" with capital I instead of "OpenAi" and "API" with all capital instead of "API" everywhere. 
```

Co-authored-by: Rick Hennigan <[email protected]>
WolframResearch#456 (comment)

Use `find Source/ -type f -exec sed -i 's/OpenAiApiCompletionURL/OpenAIAPICompletionURL/g' {} +` to minimize the change that missing something
@nhannht
Copy link
Contributor Author

nhannht commented Nov 16, 2023

If you have time can you consider checking my newest fix 🤗

@rhennigan
Copy link
Member

@nhannht I think this looks good and ready to merge, but I realized that this repository was missing the standard contributor notice used in other Wolfram repositories. I just added it here.

Can you take a moment to read that and just confirm here that you agree with the "Licensing of Contributions" part?

I'll go ahead and merge this once you confirm.

@nhannht
Copy link
Contributor Author

nhannht commented Nov 17, 2023

Thank you, I tried to read all 3 links, I can grasp at least a basic. (to be honest, this is the first time I have tried to read these kind of documents 😄)

I understood and agreed with all the terms below :
https://github.com/WolframResearch/Chatbook/blob/main/CONTRIBUTING.md?rgh-link-date=2023-11-16T15%3A11%3A16Z
image

@rhennigan rhennigan merged commit 8f97757 into WolframResearch:main Nov 20, 2023
@nhannht nhannht deleted the nhannht branch November 21, 2023 10:04
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.

3 participants