Add AiModelService for improving JabRef handling of language models (#13860) #14197
Conversation
Hey @st-rm-ng!Thank you for contributing to JabRef! Your help is truly appreciated ❤️. We have automatic checks in place, based on which you will soon get automated feedback if any of them are failing. We also use TragBot with custom rules that scans your changes and provides some preliminary comments, before a maintainer takes a look. TragBot is still learning, and may not always be accurate. In the "Files changed" tab, you can go through its comments and just click on "Resolve conversation" if you are sure that it is incorrect, or comment on the conversation if you are doubtful. Please re-check our contribution guide in case of any other doubts related to our contribution workflow. |
|
Your code currently does not meet JabRef's code guidelines. IntelliJ auto format covers some cases. There seem to be issues with your code style and autoformat configuration. Please reformat your code (Ctrl+Alt+L) and commit, then push. In special cases, consider using |
| } | ||
| } | ||
| } catch (Exception e) { | ||
| LOGGER.warn("Failed to parse models response: {}", e.getMessage()); |
There was a problem hiding this comment.
Please read our docs on Logging.
| * @param apiKey The API key for authentication | ||
| * @return A CompletableFuture containing the list of model names | ||
| */ | ||
| public CompletableFuture<List<String>> fetchModelsAsync(AiProvider aiProvider, String apiBaseUrl, String apiKey) { |
There was a problem hiding this comment.
I think there is too much asynchronous/parallelism.
What we use in JabRef, and what I think is enough to do is to actually write only synchronous methods, and if we need something to happen in background we create a new class that inherits from BackgroundTask and it calls a synchronous method.
So, you write everything synchronously, then you create a class FetchAiModelsBackgroundTask, and in the AiService you start that task. Could you implement an approach like this?
| private final HttpClient httpClient; | ||
|
|
||
| public OpenAiCompatibleModelProvider() { | ||
| this.httpClient = HttpClient.newBuilder() |
There was a problem hiding this comment.
Just in case, can you check: does this object needs to be closed?
| || aiProvider == AiProvider.GPT4ALL; | ||
| } | ||
|
|
||
| private String buildModelsEndpoint(String apiBaseUrl) { |
There was a problem hiding this comment.
Can you use a type that is more specific to URLs, rather than a raw string?
|
|
||
| @Override | ||
| public boolean supports(AiProvider aiProvider) { | ||
| // OpenAI-compatible providers: OpenAI, Mistral AI, and custom OpenAI-compatible endpoints |
There was a problem hiding this comment.
I think this comment (and potentially other docstrings) should be rewritten to sound like this:
"Mistral provides an API that has the same endpoint for fetching available models as OpenAI, thus this class is able to fetch models also from Mistral"
This is not ideal wording, but I hope you got the idea - so that person reading code would understand why these providers are called "OpenAI compatible". It's actually my mistake too, as I also haven't explained this properly
4 participants
Closes #13860
Steps to test
Mandatory checks
CHANGELOG.mdin a way that is understandable for the average user (if change is visible to the user)