Skip to content
This repository was archived by the owner on Jul 22, 2025. It is now read-only.

Conversation

@SamSaffron
Copy link
Member

@SamSaffron SamSaffron commented Feb 7, 2025

This PR introduces several enhancements and refactorings to the AI Persona and RAG (Retrieval-Augmented Generation) functionalities within the discourse-ai plugin. Here's a breakdown of the changes:

1. LLM Model Association for RAG and Personas:

  • New Database Columns: Adds rag_llm_model_id to both ai_personas and ai_tools tables. This allows specifying a dedicated LLM for RAG indexing, separate from the persona's primary LLM. Adds default_llm_id and question_consolidator_llm_id to ai_personas.
  • Migration: Includes a migration (20250210032345_migrate_persona_to_llm_model_id.rb) to populate the new default_llm_id and question_consolidator_llm_id columns in ai_personas based on the existing default_llm and question_consolidator_llm string columns, and a post migration to remove the latter.
  • Model Changes: The AiPersona and AiTool models now belong_to an LlmModel via rag_llm_model_id. The LlmModel.proxy method now accepts an LlmModel instance instead of just an identifier. AiPersona now has default_llm_id and question_consolidator_llm_id attributes.
  • UI Updates: The AI Persona and AI Tool editors in the admin panel now allow selecting an LLM for RAG indexing (if PDF/image support is enabled). The RAG options component displays an LLM selector.
  • Serialization: The serializers (AiCustomToolSerializer, AiCustomToolListSerializer, LocalizedAiPersonaSerializer) have been updated to include the new rag_llm_model_id, default_llm_id and question_consolidator_llm_id attributes.

2. PDF and Image Support for RAG:

  • Site Setting: Introduces a new hidden site setting, ai_rag_pdf_images_enabled, to control whether PDF and image files can be indexed for RAG. This defaults to false.
  • File Upload Validation: The RagDocumentFragmentsController now checks the ai_rag_pdf_images_enabled setting and allows PDF, PNG, JPG, and JPEG files if enabled. Error handling is included for cases where PDF/image indexing is attempted with the setting disabled.
  • PDF Processing: Adds a new utility class, DiscourseAi::Utils::PdfToImages, which uses ImageMagick (magick) to convert PDF pages into individual PNG images. A maximum PDF size and conversion timeout are enforced.
  • Image Processing: A new utility class, DiscourseAi::Utils::ImageToText, is included to handle OCR for the images and PDFs.
  • RAG Digestion Job: The DigestRagUpload job now handles PDF and image uploads. It uses PdfToImages and ImageToText to extract text and create document fragments.
  • UI Updates: The RAG uploader component now accepts PDF and image file types if ai_rag_pdf_images_enabled is true. The UI text is adjusted to indicate supported file types.

3. Refactoring and Improvements:

  • LLM Enumeration: The DiscourseAi::Configuration::LlmEnumerator now provides a values_for_serialization method, which returns a simplified array of LLM data (id, name, vision_enabled) suitable for use in serializers. This avoids exposing unnecessary details to the frontend.
  • AI Helper: The AiHelper::Assistant now takes optional helper_llm and image_caption_llm parameters in its constructor, allowing for greater flexibility.
  • Bot and Persona Updates: Several updates were made across the codebase, changing the string based association to a LLM to the new model based.
  • Audit Logs: The DiscourseAi::Completions::Endpoints::Base now formats raw request payloads as pretty JSON for easier auditing.
  • Eval Script: An evaluation script is included.

4. Testing:

  • The PR introduces a new eval system for LLMs, this allows us to test how functionality works across various LLM providers. This lives in /evals

@SamSaffron SamSaffron marked this pull request as draft February 7, 2025 04:40
@SamSaffron SamSaffron marked this pull request as ready for review February 12, 2025 00:55
)
end

if %w[png jpg jpeg].include?(upload.extension)
Copy link
Member

Choose a reason for hiding this comment

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

Part of my above comment, we can easily call checks for extensions if we create a helper somewhere.

Suggested change
if %w[png jpg jpeg].include?(upload.extension)
if FileHelper.ai_supported_images.include?(upload.extension)

class AiPersona < ActiveRecord::Base
# TODO remove this line 01-1-2025
self.ignored_columns = %i[commands allow_chat mentionable]
# TODO remove this line 01-10-2025
Copy link
Member

@keegangeorge keegangeorge Feb 12, 2025

Choose a reason for hiding this comment

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

Did you mean 02-10-2025? Although, this date has also passed now, should we update it to something further in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh dates ... this is D-M-Y ... generally I try to stick with that for code comments. dates are hard, maybe for comments like this we should go with October-2025, a lot less ambiguous

Comment on lines +80 to +86
get acceptedFileTypes() {
if (this.args?.allowPdfsAndImages) {
return ".txt,.md,.pdf,.png,.jpg,.jpeg";
} else {
return ".txt,.md";
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we also ensure these extensions are siteSettings.authorized_extensions_for_staff ? We can use authorizedExtensions from discourse/lib/uploads

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah this does raise issues if you half do the change, we already have a problem with txt/md which is probably more serious.

I think the right thing to do here is for the plugin to be allowed to override everything and just upload the things it wants. Not sure.

end
end

puts
Copy link
Member

Choose a reason for hiding this comment

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

trailing empty puts

Suggested change
puts

Copy link
Member Author

Choose a reason for hiding this comment

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

this was deliberate, but I will clean up more stuff in the eval system to make this stuff clearer.

@xfalcox
Copy link
Member

xfalcox commented Feb 12, 2025

Still thinking how to better add our self-hosted LLMs to evals, but this is looking great!

@SamSaffron SamSaffron merged commit 5e80f93 into main Feb 14, 2025
6 checks passed
@SamSaffron SamSaffron deleted the pdf branch February 14, 2025 01:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants