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: Add support for Mistral API models #2053

Merged
merged 27 commits into from
Mar 13, 2024
Merged

Conversation

Olyxz16
Copy link
Contributor

@Olyxz16 Olyxz16 commented Feb 28, 2024

Describe your changes

Added models mistral-tiny, mistral-small and mistral-medium from Mistral API.

Issue ticket number and link

Fixes #1991

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests. - N/A
  • I have added thorough documentation for my code.
  • I have tagged PR with relevant project labels. I acknowledge that a PR without labels may be dismissed.
  • If this PR addresses a bug, I have provided both a screenshot/video of the original bug and the working solution. - N/A

Demo

Demo

Notes

Mistral API : https://docs.mistral.ai/api/

Signed-off-by: Jared Van Bortel <jared@nomic.ai>
Signed-off-by: Olyxz16 <cedric.sazos@tutanota.com>
Signed-off-by: Olyxz16 <cedric.sazos@tutanota.com>
@manyoso
Copy link
Collaborator

manyoso commented Mar 6, 2024

@cebtenzzre look good now?

@cebtenzzre cebtenzzre self-requested a review March 6, 2024 19:28
Signed-off-by: Olyxz16 <cedric.sazos@tutanota.com>
@Olyxz16
Copy link
Contributor Author

Olyxz16 commented Mar 9, 2024

should be good, please review

Signed-off-by: Olyxz16 <cedric.sazos@tutanota.com>
Copy link
Collaborator

@manyoso manyoso left a comment

Choose a reason for hiding this comment

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

This has conflicts and needs that need to be resolved and then we'll review again.

@manyoso
Copy link
Collaborator

manyoso commented Mar 9, 2024

In general, I'm really happy to have someone working on updating these remote Models and consolidating the code for them / keeping them up2date as the remote API's change. Would be cool to have Claude support as well!

Signed-off-by: Olyxz16 <cedric.sazos@tutanota.com>

# Conflicts:
#	gpt4all-chat/chatapi.cpp
#	gpt4all-chat/modellist.cpp
@Olyxz16
Copy link
Contributor Author

Olyxz16 commented Mar 9, 2024

it's merged !
I don't have access to claude's api, if it's similar to chatgpt's, give me the endpoint and I can add it

@Olyxz16
Copy link
Contributor Author

Olyxz16 commented Mar 9, 2024

In general, I'm really happy to have someone working on updating these remote Models and consolidating the code for them / keeping them up2date as the remote API's change. Would be cool to have Claude support as well!

I plan on changing the way we handle them and use the same system as the other models

Signed-off-by: Cédric Sazos <cedric.sazos@tutanota.com>
Copy link
Collaborator

@manyoso manyoso left a comment

Choose a reason for hiding this comment

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

We have to figure out the naming convention we want and then change accordingly, but otherwise I'm happy with this. How about you @cebtenzzre ?

gpt4all-chat/modellist.cpp Show resolved Hide resolved
gpt4all-chat/modellist.cpp Show resolved Hide resolved
gpt4all-chat/modellist.cpp Show resolved Hide resolved
gpt4all-chat/modellist.cpp Show resolved Hide resolved
gpt4all-chat/modellist.cpp Show resolved Hide resolved
Signed-off-by: Cédric Sazos <cedric.sazos@tutanota.com>
@Olyxz16
Copy link
Contributor Author

Olyxz16 commented Mar 11, 2024

So what should I do next ?
If you want to implement a prefix for the file names and get the previous names back, we will have to implement something to store the name of the api model name.

@manyoso
Copy link
Collaborator

manyoso commented Mar 12, 2024

So what should I do next ? If you want to implement a prefix for the file names and get the previous names back, we will have to implement something to store the name of the api model name.

Going to have @cebtenzzre look at this tomorrow but after we come up with a solution for the prefix file names and we figure out the issues below, then we're hopefully merging this week

gpt4all-chat/chatapi.h Show resolved Hide resolved
gpt4all-chat/chatllm.cpp Outdated Show resolved Hide resolved
Signed-off-by: Cédric Sazos <cedric.sazos@tutanota.com>
@manyoso
Copy link
Collaborator

manyoso commented Mar 12, 2024

NOTE: This very closely related PR that I'm closing due to conflicts... but putting it here for posterity: #1692

gpt4all-chat/chatapi.cpp Outdated Show resolved Hide resolved
gpt4all-chat/modellist.cpp Show resolved Hide resolved
Signed-off-by: Cédric Sazos <cedric.sazos@tutanota.com>
@Olyxz16
Copy link
Contributor Author

Olyxz16 commented Mar 12, 2024

I do agree with @cebtenzzre about the naming convention of the remote model files. But the issue remains : where do we first store the name of the remote model, since it's different from the display name and the filename ?
I think we should store the remote models the same way we do with other models instead of hardcoding them, but again that would mean setting the model name somewhere into that.

Signed-off-by: Cédric Sazos <cedric.sazos@tutanota.com>
Signed-off-by: Cédric Sazos <cedric.sazos@tutanota.com>
Signed-off-by: Cédric Sazos <cedric.sazos@tutanota.com>
@Olyxz16
Copy link
Contributor Author

Olyxz16 commented Mar 13, 2024

I fixed the backward compatibility issue by checking for remaining .txt file when loading the models. I then transform them into the new .rmodel format. The process then continues like before

gpt4all-chat/chatllm.cpp Outdated Show resolved Hide resolved
gpt4all-chat/chatllm.h Outdated Show resolved Hide resolved
gpt4all-chat/download.cpp Outdated Show resolved Hide resolved
gpt4all-chat/modellist.cpp Outdated Show resolved Hide resolved
gpt4all-chat/modellist.cpp Outdated Show resolved Hide resolved
cebtenzzre and others added 7 commits March 13, 2024 17:45
Signed-off-by: Jared Van Bortel <jared@nomic.ai>
Signed-off-by: Cédric Sazos <cedric.sazos@tutanota.com>
Signed-off-by: Cédric Sazos <cedric.sazos@tutanota.com>
Signed-off-by: Cédric Sazos <cedric.sazos@tutanota.com>
Signed-off-by: Cédric Sazos <cedric.sazos@tutanota.com>
…x-1991

Signed-off-by: Cédric Sazos <cedric.sazos@tutanota.com>

# Conflicts:
#	gpt4all-chat/chatapi.cpp
#	gpt4all-chat/chatllm.cpp
Signed-off-by: Jared Van Bortel <jared@nomic.ai>
@cebtenzzre
Copy link
Member

@Olyxz16 Heh sorry, manyoso suggested that I make the style changes for you but apparently you were already working on it, so we duplicated some work. Looks like it came out OK in the merge.

Signed-off-by: Jared Van Bortel <jared@nomic.ai>
@Olyxz16
Copy link
Contributor Author

Olyxz16 commented Mar 13, 2024

@Olyxz16 Heh sorry, manyoso suggested that I make the style changes for you but apparently you were already working on it, so we duplicated some work. Looks like it came out OK in the merge.

Yeah I saw your commit after my changes, no worries you changed more things than I did anyway

@manyoso
Copy link
Collaborator

manyoso commented Mar 13, 2024

looks like all we're missing is another conflict resolution and then this is ready to merge?

Signed-off-by: Cédric Sazos <cedric.sazos@tutanota.com>

# Conflicts:
#	gpt4all-backend/llama.cpp-mainline
#	gpt4all-chat/chatllm.h
#	gpt4all-chat/modellist.cpp
@Olyxz16
Copy link
Contributor Author

Olyxz16 commented Mar 13, 2024

Just merged, there was conflicts on the backend I didn't know how to solve, so I just chose the branch responsible for the changes

Copy link
Member

@cebtenzzre cebtenzzre left a comment

Choose a reason for hiding this comment

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

Hold on, the merge isn't quite right.

Signed-off-by: Jared Van Bortel <jared@nomic.ai>
Copy link
Member

@cebtenzzre cebtenzzre left a comment

Choose a reason for hiding this comment

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

Fixed.

@manyoso
Copy link
Collaborator

manyoso commented Mar 13, 2024

Fixed.

I'm still seeing, "This branch cannot be rebased due to conflicts"

@cebtenzzre cebtenzzre merged commit 2c0a660 into nomic-ai:main Mar 13, 2024
2 checks passed
@cebtenzzre
Copy link
Member

This branch cannot be rebased due to conflicts

Probably because rebasing merges isn't trivial. I just squashed it to keep things clean - one can click the PR link to see the history of this specific change.

@manyoso
Copy link
Collaborator

manyoso commented Mar 13, 2024

image

@manyoso
Copy link
Collaborator

manyoso commented Mar 13, 2024

we need a check for the prefix of the old "chatgpt-" when converting the .rmodel and probably 'nomic-' too?

@cebtenzzre
Copy link
Member

we need a check for the prefix of the old "chatgpt-" when converting the .rmodel and probably 'nomic-' too?

It looks like Nomic Embed is not even converted and still uses the old name.

@manyoso
Copy link
Collaborator

manyoso commented Mar 13, 2024

we need a check for the prefix of the old "chatgpt-" when converting the .rmodel and probably 'nomic-' too?

It looks like Nomic Embed is not even converted and still uses the old name.

See #2119

I'm leaving the nomic one now as I think there is more that needs to be changed and I'll want to test to make sure it works

jacoobes pushed a commit to jacoobes/gpt4all that referenced this pull request Mar 18, 2024
Signed-off-by: Jared Van Bortel <jared@nomic.ai>
Signed-off-by: Cédric Sazos <cedric.sazos@tutanota.com>
Co-authored-by: Jared Van Bortel <jared@nomic.ai>
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.

[Feature] Add support for Mistral API
3 participants