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

Customizable AI templates #11884

Merged
merged 10 commits into from
Oct 30, 2024
Merged

Customizable AI templates #11884

merged 10 commits into from
Oct 30, 2024

Conversation

InAnYan
Copy link
Collaborator

@InAnYan InAnYan commented Oct 6, 2024

Now, all LLM templates can be customized by users. Previously, only the system message was customizable in a limited way.

I used Apache Velocity template engine.

image
image

Mandatory checks

- [ ] Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
- [ ] Tests created for changes (if applicable)

# Conflicts:
#	src/main/java/module-info.java
#	src/main/java/org/jabref/gui/preferences/ai/AiTab.java
#	src/main/java/org/jabref/logic/ai/AiService.java
#	src/main/resources/l10n/JabRef_en.properties
@koppor
Copy link
Member

koppor commented Oct 6, 2024

Side comment: This refs JabRef#392, where we first wanted Apache Velocity - but then wanted Thymeleaf (and did not want Apache FreeMarker).

@calixtus
Copy link
Member

calixtus commented Oct 6, 2024

I think it I necessary now to finally create an ADR now for the template engine. Please create a simple one, with your decision drivers for velocity and we can fill in the others for the next dev-call to decide on it together.

I think we need to decide on it, since we should not have multiple templating engines running in favor of resources.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Minor comments.

Regarding the wording...

grafik

Should it be

  • User message for a summary of a chunk of text
  • User message for a combinding chunk summaries

?

DOCUMENT:
$document

OVERVIEW:""",
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand that - this should be generated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

About that. There are two ways of asking AI to generate text:

  1. In form of chat: a list of messages. For this situation we need both system + user message templates.
  2. In form of text that should be completed. This is only one template + I've seen that this approach is typically chosen for summarization. And I think it's easier.

For the summarization I chose 2nd approach. That is why you are seeing "OVERVIEW" and "FINAL OVERVIEW"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because without those words AI won't understand what should it do

SUMMARIES:
$summaries

FINAL OVERVIEW:"""
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand that - the FINAL OVERVIEW should be generated? Why is it part of the prompt?

Comment on lines 19 to 28
return switch (this) {
case CHATTING_SYSTEM_MESSAGE ->
Localization.lang("System message for chatting");
case CHATTING_USER_MESSAGE ->
Localization.lang("User message for chatting");
case SUMMARIZATION_CHUNK ->
Localization.lang("Summarization chunk");
case SUMMARIZATION_COMBINE ->
Localization.lang("Summarization combine");
};
Copy link
Member

Choose a reason for hiding this comment

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

We can actually define fields in enums, so here we can define a field called localizedName and define the relevant constructor. After that we can write something like this: CHATTING_SYSTEM_MESSAGE(Localization.lang("System message for chatting"))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But I'm not sure, if the names are properties of templates, and especially localized ones

Copy link
Member

Choose a reason for hiding this comment

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

If that is the case we can do something else, but if all strings are static having it as a field is better IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Just call Localization.lang in the enum. Why not? -- The initialization of the enum should take care about the localiaztion. When using toString, the stored localized string is returned.

I know this will cause troubles when users change the UI language. Since language switch is not done that often, I accept this.


I think, a related discussion was going on at #8304 and ADR-0023.

# Conflicts:
#	src/main/java/org/jabref/logic/ai/summarization/GenerateSummaryTask.java
#	src/main/java/org/jabref/logic/ai/summarization/SummariesService.java
#	src/main/resources/l10n/JabRef_en.properties
@koppor koppor mentioned this pull request Oct 12, 2024
@koppor
Copy link
Member

koppor commented Oct 16, 2024

I think it I necessary now to finally create an ADR now for the template engine.

Coming from #11900, Velocity was the right choice. We need to improve the ADR itself, but it is good enough to show that this was the right choice.

@koppor
Copy link
Member

koppor commented Oct 28, 2024

@InAnYan Can you resolve the conflicts? I think, then, this can go in - and JabRef/user-documentation#519 can also be merged?

@InAnYan
Copy link
Collaborator Author

InAnYan commented Oct 28, 2024

Cool, the ADR is merged!

I'll fix everything, but in the evening

# Conflicts:
#	src/main/java/org/jabref/gui/preferences/ai/AiTabViewModel.java
#	src/main/java/org/jabref/logic/ai/AiDefaultPreferences.java
Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Comments on naming.

Moreover, can you change from dropdown to tabs?

JabRef uses tabs as UX pattern for options. (Moreover, it is one click for change to another template - for instanc,e when trying to make them consistent, comparing them, ... this is useful)

src/main/resources/l10n/JabRef_en.properties Outdated Show resolved Hide resolved
src/main/resources/l10n/JabRef_en.properties Show resolved Hide resolved
@koppor koppor added this pull request to the merge queue Oct 30, 2024
Merged via the queue into JabRef:main with commit 709386a Oct 30, 2024
27 of 28 checks passed
@koppor koppor deleted the ai-templates branch October 30, 2024 09:27
@ThiloteE ThiloteE mentioned this pull request Nov 6, 2024
4 tasks
@ThiloteE ThiloteE added the AI Related to AI Chat/Summarization label Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AI Related to AI Chat/Summarization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants