-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
AI chatting functionality #11430
AI chatting functionality #11430
Conversation
On my machine, when I run "checkstyleMain" from Gradle commands, I have no such errors. It has successfully generated for me a list of violations |
Then it seems reviewdog seems to have an outdated version of checkstyle |
src/main/java/org/jabref/logic/ai/embeddings/EmbeddingsGenerationTaskManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/ai/chathistory/BibDatabaseChatHistory.java
Outdated
Show resolved
Hide resolved
I updated the checkstyle reviewdog action to a newer version so the checkstyle works now again and can parse your code :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some quick notes after i have skimmed the changes.
|
||
this.onFailure(e -> { | ||
throw new RuntimeException(e); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be aware that the whole indexing architecture will be refactored when the lucene search project is integrated.
This will produce merge conflicts. The less changes in the search package, the better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, this change can be reverted.
NEVER use new RuntimeException(e);
. This tears down the whole application and all data is lost. Is this error really that severe? No, it is not.
@@ -42,6 +42,7 @@ | |||
|
|||
/** | |||
* Indexes the text of PDF files and adds it into the lucene search index. | |||
* Also generates embeddings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will cause merge conflicts when lucene search is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, the whole change can be reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I clicked the revert button.
|
||
/* AI chat style */ | ||
-jr-ai-message-user: -jr-accent; | ||
-jr-ai-message-ai: -jr-accent; | ||
|
||
/* MarkdownViewer settings */ | ||
-mdfx-font-color: black; | ||
-mdfx-link-color: blue; | ||
-mdfx-border-color-1: #888; | ||
|
||
-mdfx-bg-color-1: #ccc; | ||
-mdfx-bg-color-2: #ddd; | ||
-mdfx-bg-color-3: #eee; | ||
|
||
-mdfx-bq-color-border: #4488cc; /* the blockquote's border color */ | ||
-mdfx-bq-color-background: #0000ff0c; /* the blockquote's background color */ | ||
} | ||
|
||
.markdown-text { | ||
-fx-font-family: "System"; | ||
-fx-font-size: 13; | ||
} | ||
|
||
.markdown-italic { | ||
-fx-font-style: italic; | ||
} | ||
|
||
.markdown-bold { | ||
-fx-font-weight: bold; | ||
} | ||
|
||
.markdown-strikethrough { | ||
-fx-strikethrough: true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be an artifact after ADR34
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That mean,s these .markdown-*
and maybe -mdfx-*
items can be removed.
src/main/java/org/jabref/gui/ai/components/privacynotice/PrivacyNoticeComponent.java
Outdated
Show resolved
Hide resolved
Use as many threads as there are physical cores. Using LLMs, that's the most performant in my personal tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went half thorugh. Will continue later
General comment:
NEVER use new RuntimeException(e);. This tears down the whole application and all data is lost. Is this error really that severe? No, it is not.
Minr comments:
I reverted changes in files which have no impact on the concrete functionality.
I also reverted level@org.jabref.logic.ai.embeddings.AiIngestor = trace
, because this is for development only, not for a distribution of JabRef.
I also made code suggestions. You can collect them and commit them togehter (and not create several commits) - https://docs.github.com/de/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request
|
||
/* AI chat style */ | ||
-jr-ai-message-user: -jr-accent; | ||
-jr-ai-message-ai: -jr-accent; | ||
|
||
/* MarkdownViewer settings */ | ||
-mdfx-font-color: black; | ||
-mdfx-link-color: blue; | ||
-mdfx-border-color-1: #888; | ||
|
||
-mdfx-bg-color-1: #ccc; | ||
-mdfx-bg-color-2: #ddd; | ||
-mdfx-bg-color-3: #eee; | ||
|
||
-mdfx-bq-color-border: #4488cc; /* the blockquote's border color */ | ||
-mdfx-bq-color-background: #0000ff0c; /* the blockquote's background color */ | ||
} | ||
|
||
.markdown-text { | ||
-fx-font-family: "System"; | ||
-fx-font-size: 13; | ||
} | ||
|
||
.markdown-italic { | ||
-fx-font-style: italic; | ||
} | ||
|
||
.markdown-bold { | ||
-fx-font-weight: bold; | ||
} | ||
|
||
.markdown-strikethrough { | ||
-fx-strikethrough: true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That mean,s these .markdown-*
and maybe -mdfx-*
items can be removed.
src/main/java/org/jabref/logic/ai/chat/AiChatLanguageModel.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/ai/chat/AiChatLanguageModel.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/ai/embeddings/AiEmbeddingsManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/ai/embeddings/AiEmbeddingsManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/ai/embeddings/AiEmbeddingsManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/ai/embeddings/AiEmbeddingsManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/ai/embeddings/AiEmbeddingModel.java
Outdated
Show resolved
Hide resolved
Please also test with a shared SQL database. See https://docs.jabref.org/collaborative-work/sqldatabase for details. Ensure that autosave to a local .tex file is activated. i assume that either a check for database location == LOCAL -has to be added -- or another storage path for the chat history used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last comments :)
Not big ones, just some code things so that others could understand it more if they were about to maintain it.
I am worried about the shutdown. To get this fixed, I think, we need to add LOGGER.trace statements at org.jabref.gui.LibraryTab#onClosed
and other places where a shutdown happens - to see which shutdowns are triggered and which are not happening.
src/main/java/org/jabref/gui/ai/RegenerateEmbeddingsAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/ai/chathistory/BibDatabaseChatHistory.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/ai/chathistory/BibDatabaseChatHistory.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/ai/chathistory/BibDatabaseChatHistory.java
Outdated
Show resolved
Hide resolved
tabs.add(new DeprecatedFieldsTab(databaseContext, libraryTab.getSuggestionProviders(), undoManager, dialogService, preferencesService, stateManager, themeManager, libraryTab.getIndexingTaskManager(), bibEntryTypesManager, taskExecutor, journalAbbreviationRepository)); | ||
tabs.add(new OtherFieldsTab(databaseContext, libraryTab.getSuggestionProviders(), undoManager, dialogService, preferencesService, stateManager, themeManager, libraryTab.getIndexingTaskManager(), bibEntryTypesManager, taskExecutor, journalAbbreviationRepository)); | ||
tabs.add(new RequiredFieldsTab(databaseContext, libraryTab.getSuggestionProviders(), undoManager, dialogService, preferencesService, stateManager, themeManager, libraryTab.getIndexingTaskManager(), libraryTab.getEmbeddingsGenerationTaskManager(), bibEntryTypesManager, taskExecutor, journalAbbreviationRepository)); | ||
tabs.add(new ImportantOptionalFieldsTab(databaseContext, libraryTab.getSuggestionProviders(), undoManager, dialogService, preferencesService, stateManager, themeManager, libraryTab.getIndexingTaskManager(), libraryTab.getEmbeddingsGenerationTaskManager(), bibEntryTypesManager, taskExecutor, journalAbbreviationRepository)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter libraryTab.getEmbeddingsGenerationTaskManager(
is required for the AiTab
only. Remove everywhere else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's required for PreviewPanel
or PreviewTab
(#11430 (comment))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ImportantOptionalFieldsTab
do not have a preivew. Therefore, passing the whole EmbeddingsGenerationTaskManager
is strange.
@calixtus A) Pass EmbeddingsGenerationTaskManager
down to all tabs or B) use ´Injector.instantiateModelOrService(EmbeddingsGenerationTaskManager.class)` when really needed?
comment to A): This is required, because we have abstract class FieldsEditorTab extends EntryEditorTab implements OffersPreview {
. And each preview offers drag'n'drop of files. Thus, file events should be handled.
I see more options (with more work)
- C): Move
implements OffersPreview
to the tabs really offering a preview and try to pass the EmbeddingsGenerationTaskManager only to those - D) Use "the" event bus to handle file events and have
EmbeddingsGenerationTaskManager
subscribing to these file changes. This way, we would decouple the places and have "only"org.jabref.gui.externalfiles.ExternalFilesEntryLinker#moveFilesToFileDirRenameAndAddToEntry
knowing the task managers - and not somewhere in the UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Option D seems very interesting)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Option D seems very interesting)
I like most. But one needs to take care to jump from the JavaFX thread to a Background Thread.
Hints:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, we did that. Thus. closing this.
Maybe, we need a setting. If the thing takes 1 hour on a "normal" machine to create embeddings and the user cannot work in parallel, this is a no-go in my oppinion. The PC should be usable without any signifcant lags. I know that Linux is a better in multi tasking. However, there are still persons using Windows. Thus, JabRef should also be useable for Windows users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more comments maybe (foudn another firefox window)
src/main/java/org/jabref/gui/preferences/ai/AiTabViewModel.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/entryeditor/aichattab/AiChatTabWorking.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/entryeditor/aichattab/AiChatTabWorking.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/entryeditor/aichattab/AiChatTabWorking.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/ai/embeddings/MVStoreEmbeddingStore.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/ai/chat/AiChatLanguageModel.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/ai/chat/AiChatLanguageModel.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/ai/chat/AiChatLanguageModel.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/logic/ai/chat/AiChatLanguageModel.java
Outdated
Show resolved
Hide resolved
I refactored API key handling - but |
what kind of mock issues? I ran the test, and it seems okay, or am I doing something wrong? |
yeah seems to be all green now |
For me, it is OK. More changes can be done in a follow-up PR. We collected issues for follow up at https://github.com/InAnYan/jabref/milestone/12. |
I think, the last commit is OK. Even if I fear that changing the prompt only will lead to a complete re-indexing, which is not desired at all! |
This reverts commit 5fa3bb5.
The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build. |
Congratulations, Ruslan. This was a big one (for you as well as for JabRef)! |
Screenshots of the UI changes
Old chat style:
New chat style:
Summarization:
AI preferences:
TODOs
(Update by koppor)
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if applicable)