Add AiModelService for improving JabRef handling of language models (#13860)

[st-rm-ng]

Closes #13860

• This pull request adds an AiModelService to improve how JabRef handles and manages language models for AI features.
• Created new AiModelService class to centralize AI model management

Steps to test

./gradlew :jablib:test --tests "org.jabref.logic.ai.models.*Test"

Mandatory checks

• [x] I own the copyright of the code submitted and I license it under the MIT license
• [x] I manually tested my changes in running JabRef (always required)
• [x] I added JUnit tests for changes (if applicable)
• [ ] I added screenshots in the PR description (if change is visible to the user)
• [ ] I described the change in CHANGELOG.md in a way that is understandable for the average user (if change is visible to the user)
• [/] I checked the user documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request updating file(s) in https://github.com/JabRef/user-documentation/tree/main/en.

[github-actions[bot]]

Hey @st-rm-ng!

Thank you for contributing to JabRef! Your help is truly appreciated :heart:.

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.

[st-rm-ng]

Hello @koppor and @InAnYan,

this is just a Draft PR, and it's not ready to be tested or anything. Think of this as my initial attempt or establishment to view all changes. I will let you know when it will be in ready for review state.

Thank you very much for your quick response to this PR 😄

[st-rm-ng]

The threading thing looks complicated. There at least should be a note why a "synchronous" nested thread-spawning thing is nested in a Background-Task.

Not sure if this is the call for using RxJava to enable easy parallel processing: https://github.com/ReactiveX/RxJava#parallel-processing

The reason for this complexity is to implement a timeout mechanism that we need to avoid blocking the BackgroundTask thread indefinitely. You are right, this solution is maybe overly complicated and from my perspective it would be maybe better to use HttpClient.sendAsync() and CompletableFuture.orTimeout() to avoid nested thread spawning.

Regarding RxJava - I think that in this case I am only fetching models from one provider at a time and maybe it would be better for the future to fetch models from multiple providers in parallel using RxJava. Maybe something like Observable.fromIterable(providers).flatMap(...).timeout(...) which would look more elegant.

@koppor Should I use RxJava approach instead? What would be your advice?

[Siedlerchr]

The threading thing looks complicated. There at least should be a note why a "synchronous" nested thread-spawning thing is nested in a Background-Task. Not sure if this is the call for using RxJava to enable easy parallel processing: https://github.com/ReactiveX/RxJava#parallel-processing

The reason for this complexity is to implement a timeout mechanism that we need to avoid blocking the BackgroundTask thread indefinitely. You are right, this solution is maybe overly complicated and from my perspective it would be maybe better to use HttpClient.sendAsync() and CompletableFuture.orTimeout() to avoid nested thread spawning.

Regarding RxJava - I think that in this case I am only fetching models from one provider at a time and maybe it would be better for the future to fetch models from multiple providers in parallel using RxJava. Maybe something like Observable.fromIterable(providers).flatMap(...).timeout(...) which would look more elegant.

@koppor Should I use RxJava approach instead? What would be your advice?

For timeouts you could leverage the functionality of the Completable Future https://www.baeldung.com/java-completablefuture-timeout I don't think rxjava would be needed here

[koppor]

The threading thing looks complicated. There at least should be a note why a "synchronous" nested thread-spawning thing is nested in a Background-Task. Not sure if this is the call for using RxJava to enable easy parallel processing: https://github.com/ReactiveX/RxJava#parallel-processing

The reason for this complexity is to implement a timeout mechanism that we need to avoid blocking the BackgroundTask thread indefinitely. You are right, this solution is maybe overly complicated and from my perspective it would be maybe better to use HttpClient.sendAsync() and CompletableFuture.orTimeout() to avoid nested thread spawning.

Regarding RxJava - I think that in this case I am only fetching models from one provider at a time and maybe it would be better for the future to fetch models from multiple providers in parallel using RxJava. Maybe something like Observable.fromIterable(providers).flatMap(...).timeout(...) which would look more elegant.

@koppor Should I use RxJava approach instead? What would be your advice?

For timeouts you could leverage the functionality of the Completable Future https://www.baeldung.com/java-completablefuture-timeout I don't think rxjava would be needed here

@st-rm-ng please follow that advice.

[InAnYan]

Okay, I only reviewed the code, but I find it very clear and easy to read. But may I ask this question: why there is at all some complicated logic for fetching the models? Why do you need to create a fetch thread and check that it's alive?

I think 1 BackgroundTask (which you already have) that is executed with a TaskExecutor on the start of JabRef is enough. And in case user wants to refresh, you can start that task when preferences window is open, or add a ⟳ button near the chat model field

[InAnYan]

Sorry, maybe I miss something, but I still don't get why implementing timeout is that hard and how it can block the background tasks? (I think we use a thread pool)

Here is an example how to send http request with timeout that ChatGPT generated me:

import java.net.URI;
import java.net.http.HttpClient;
import java.net.http.HttpRequest;
import java.net.http.HttpResponse;
import java.time.Duration;

public class SimpleHttp {
    public static void main(String[] args) throws Exception {
        HttpClient client = HttpClient.newBuilder()
                .connectTimeout(Duration.ofSeconds(5))  // connection timeout
                .build();

        HttpRequest request = HttpRequest.newBuilder()
                .uri(new URI("https://example.com"))
                .timeout(Duration.ofSeconds(3))        // request timeout
                .GET()
                .build();

        HttpResponse<String> response = client.send(request, HttpResponse.BodyHandlers.ofString());
        System.out.println(response.body());
    }
}

When it reaches timeout, an exception will be thrown.

BackgroundTask may process it by (if I recall correctly) task.onError(e -> {...})

[InAnYan]

Sorry, maybe I just don't know something about Java

[koppor]

@InAnYan Thank you for pointing out. Your code should solve the issue. @st-rm-ng Please incorporate that idea into your solution!

[st-rm-ng]

Thank you for advices @Siedlerchr, @koppor and @InAnYan. I've removed redundant FetchThread timeout mechanism for simpler synchronous model fetching and I've used built-in timeout for HTTP Client fetching. I didn't use CompletableFuture and it's just simply relying on HTTP Client timeout. Is this what you had in mind @InAnYan?

[koppor]

@st-rm-ng please request a review after all tests go through (or ask for help if you tried hard to fix tests and you did not manage). Thank you.

[st-rm-ng]

@koppor there are 13 Unit tests that are failing are failing even on main branch and are not related to my code when I run full Gradle Build locally

[st-rm-ng]

@koppor I've fixed all GitHub checks, but I still can't pass those 13 Unit tests when creating full local build

[koppor]

@koppor I've fixed all GitHub checks, but I still can't pass those 13 Unit tests when creating full local build

As long as they run on GitHub, it should be fine... I assume you are on Windows?

[st-rm-ng]

@koppor I've fixed all GitHub checks, but I still can't pass those 13 Unit tests when creating full local build

As long as they run on GitHub, it should be fine... I assume you are on Windows?

I am building on Mac JDK 24 Oracle Temurin

[koppor]

@st-rm-ng Any interest to continue here or should we free this issue for another contributor?

[st-rm-ng]

@koppor I've made modifications to use Nullmarked and Nullable as you hinted me. Is this what you had in your mind?

[koppor]

Currently no user-visible change, is it?\n\nI think, the model list in the preferences should be updated, shouldn't hey? Did I miss something?

@st-rm-ng what's the answer for that?

[st-rm-ng]

Currently no user-visible change, is it?\n\nI think, the model list in the preferences should be updated, shouldn't hey? Did I miss something?

@st-rm-ng what's the answer for that?

I haven't connected this back-end code to UI yet, still working on it

[koppor]

@koppor I've made modifications to use Nullmarked and Nullable as you hinted me. Is this what you had in your mind?

Nice one thank you. Looking forward for the update.