Discuss: Unify translate operations over ai

Created on 4 October 2024, 3 months ago

Problem/Motivation

🌱 Discuss: Translation operator type Active has introduced a new operation type "Translate text". Since this is a new operation, other translation-related parts (ai_translate, translate plugin of ai_ckeditor) use "Chat" operation, which exists for a long time.

As a result, there are at least 3 places to configure translation-related parameters:

  1. Translate text provider in AI settings (no bundled provider supports the new operation at the moment)
  2. AI translate settings (twig-based prompt that can be altered by third parties)
  3. Hard-coded translation prompt in CKEditor translate plugin

This may confuse both users and developers.

Proposed resolution

Have a single "point of responsibility" for translation purposes. This point should

  1. Have "text to translate" as a single required input
  2. Have a set of translation contexts (from and to languages in the form of ISO code or free name, provider id, contexts provided/altered by third parties) as an optional parameter
  3. Have "translated text" as output

Location of this point of responsibility is TBD

Remaining tasks

Decide what "translation point of responsibility" is. A service? A trait that can be then used by AI providers? AiProviderClientBase methods?

If it's a service, then what module this service belongs to (ai or ai_translate)? In case it's ai_translate, module description in ai_translate.info.yml should be changed from "one click translation" to "translation-related functionality"

📌 Task
Status

Active

Version

1.0

Component

Code

Created by

🇧🇬Bulgaria valthebald Sofia

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @valthebald
  • 🇩🇪Germany marcus_johansson

    I think that if this service should exist it has to exist in the ai_translate module, because if we start adding combinations of operation types based on what they solve rather on how they solve it and want to be consistent, there will probably be 100s of combinations in the AI core module, which will be impossible to manage.

    Having a dedicated third party module or third party service for that makes more sense in that case.

  • Merge request !210Resolve #3478721 "Unify translate" → (Open) created by valthebald
  • Pipeline finished with Failed
    2 months ago
    Total: 347s
    #315215
  • Pipeline finished with Failed
    2 months ago
    Total: 175s
    #315653
  • 🇬🇧United Kingdom scott_euser

    Adding related issue; depending on where this issue lands, we could leverage ai_translate more.

  • Pipeline finished with Failed
    2 months ago
    Total: 165s
    #315682
  • Pipeline finished with Failed
    2 months ago
    Total: 160s
    #315786
  • Pipeline finished with Failed
    about 2 months ago
    Total: 169s
    #318331
  • Pipeline finished with Failed
    about 2 months ago
    Total: 188s
    #320603
  • 🇩🇪Germany marcus_johansson

    @valthebald - code review is done from my side, the ai_translate.settings check and the setChatSystemRole method is important. Rest is optional if you think they are improvements, otherwise just skip.

    I'll let Frederick do the feature review since he knows the module better, but from what I see in the code, it mostly works the same.

  • 🇩🇪Germany marcus_johansson

    Oh also the PHPCS issues needs to be fixed.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 169s
    #321664
  • Pipeline finished with Failed
    about 2 months ago
    Total: 175s
    #321668
  • 🇧🇬Bulgaria valthebald Sofia

    Fixed most of the comments (2 left are for the future).
    Also, brought back editing of default translation prompt (patch to have per-language left it hidden) and display of default prompt that has a condition

  • Pipeline finished with Failed
    about 2 months ago
    Total: 1322s
    #321676
  • Pipeline finished with Failed
    about 2 months ago
    Total: 165s
    #321691
  • Pipeline finished with Failed
    about 2 months ago
    Total: 815s
    #321708
  • Pipeline finished with Failed
    about 2 months ago
    Total: 182s
    #321722
  • First commit to issue fork.
  • Pipeline finished with Success
    about 2 months ago
    #331178
  • Pipeline finished with Success
    about 2 months ago
    #331196
  • 🇫🇷France nicolasgraph Strasbourg

    Thanks @valthebald for this MR!

    I pushed and revert a commit in favour of this comments:

    1. In ai_translate.install, is ai_translate_requirements() still required? I guess not.
    2. In ai_translate.module, _ai_translate_check_default_provider_and_model() seems also outdated to me.
    3. In AiTranslateForm.php, line 114, $default_model = $this->providerManager->getSimpleDefaultProviderOptions('chat'); should not use the chat provider type anymore.
  • 🇬🇧United Kingdom scott_euser

    I added some feedback as well after testing this in Fallback to ai_translate configuration Active
    There are also some comments from Marcus to be addressed still

    In general though it seems like the right direction, and is really helpful to have, thank you for the efforts here!

Production build 0.71.5 2024