-
-
Notifications
You must be signed in to change notification settings - Fork 102
[Agent] Support translations for system input processor #514
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
Conversation
7580100
to
ecddc1f
Compare
src/agent/tests/InputProcessor/SystemPromptInputProcessorTest.php
Outdated
Show resolved
Hide resolved
src/agent/tests/InputProcessor/SystemPromptInputProcessorTest.php
Outdated
Show resolved
Hide resolved
Can we please make the translator configurable in the bundle? Thanks |
e99f400
to
b602f56
Compare
so, what we just discussed as config structure: system_prompt:
prompt: '...',
enable_translation: true|false, # default: true if translator is present
translation_domain: '...', # default: "messages"
include_tools: true|false # default same as now |
@chr-hertel I feel like this is kinda redundant, you could have only one key by allowing |
You are right, but it's some kind of a hidden feature. It's not clear, that string or null means, the translator is enabled. So lets go with @chr-hertel proposal |
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.
Speaking of symfony/translation-contracts, there is a TranslatableInterface.
Not sure if it would be better to have
public function __construct(
private \Stringable|string|Translatable $systemPrompt,
private ?ToolboxInterface $toolbox = null,
private ?TranslatorInterface $translator = null,
private LoggerInterface $logger = new NullLogger(),
) {
if ($systemPrompt instanceof Translatable && null === TranslatorInterface) {
throw new InvalidArgumentException(...);
}
}
private function translateIfEnabled(): string
{
if ($systemPrompt instance Translatable) {
return $systemPrompt->trans($this->translator);
}
return (string) $systemPrompt;
}
And the AiBundle
if ($config['prompt']['enable_translation']) {
if (!class_exists(TranslatableMessage::class) {
throw new InvalidArgumentException('Please install symfony/translation');
}
$prompText = new TranslatableMessage($config['prompt']['text'], domain: $config['prompt']['translation_domain']);
} else {
$prompText = $config['prompt']['text'];
}
...
->setArguments([
$prompText,
$includeTools ? new Reference('ai.toolbox.'.$name) : null,
new Reference('translator', ContainerInterface::NULL_ON_INVALID_REFERENCE),
new Reference('logger', ContainerInterface::IGNORE_ON_INVALID_REFERENCE),
])
This can also be done in another pr
Hmm, I'm not super convinced about using |
|
||
private function translateIfEnabled(): string | ||
{ | ||
if ($this->enableTranslation && !$this->translator) { |
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.
Shall we do this validation in the constructor?
We could argue the opposite. Currently we're adding complexity in SystemPromptInputProcessor which has to be aware about
Such things should be "prompt" knownledge rather than SystemPromptInputProcessor one. Quoting the original PR symfony/symfony#37670
TranslatableMessage is a way to avoid putting this knownledge inside the SystemPromptInputProcessor It's also a way to allow using translation parameters. |
So that system prompts can be written in any locale
It took time, but here we go, this is in now. Thank you very much @paulinevos. |
Documents the new system prompt translation functionality added in PR #514, including: - Configuration options for enabling translation and setting custom domains - Usage examples and integration patterns - Advanced examples with service factories and testing approaches - Performance optimization strategies with caching 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
@OskarStark would you be interested with a Poc with translatableMessage ? |
Why not, shouldn't be to much code, right? |
…dation of bundle options (chr-hertel) This PR was merged into the main branch. Discussion ---------- [AI Bundle] Fix validation on system prompt translation validation of bundle options | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | Docs? | no | Issues | | License | MIT Follow up #514 <img width="1260" height="632" alt="image" src="https://github.com/user-attachments/assets/16ae5600-a35f-45db-a39f-39a5adc5ab64" /> Commits ------- 5be7070 Fix validation on system prompt translation validation of bundle options
…Processor` (VincentLanglet) This PR was squashed before being merged into the main branch. Discussion ---------- [Agent] Allow translatable prompts in `SystemPromptInputProcessor` | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes/no | Docs? | no <!-- required for new features --> | Issues | Fix #... | License | MIT This is a follow up of #514 with a suggestion of using TranslatableMessage instance for prompts rather than the need in SystemPromptInputProcessor constructor of - A boolean enableTranslation - A domain Since we're passing a TranslatableMessage this also allow to use translation parameters. cc `@OskarStark` that was the suggestion in #514 (comment) Commits ------- 868581f [Agent] Allow translatable prompts in `SystemPromptInputProcessor`
…Processor` (VincentLanglet) This PR was squashed before being merged into the main branch. Discussion ---------- [Agent] Allow translatable prompts in `SystemPromptInputProcessor` | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes/no | Docs? | no <!-- required for new features --> | Issues | Fix #... | License | MIT This is a follow up of symfony/ai#514 with a suggestion of using TranslatableMessage instance for prompts rather than the need in SystemPromptInputProcessor constructor of - A boolean enableTranslation - A domain Since we're passing a TranslatableMessage this also allow to use translation parameters. cc `@OskarStark` that was the suggestion in symfony/ai#514 (comment) Commits ------- 868581f4 [Agent] Allow translatable prompts in `SystemPromptInputProcessor`
…Processor` (VincentLanglet) This PR was squashed before being merged into the main branch. Discussion ---------- [Agent] Allow translatable prompts in `SystemPromptInputProcessor` | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes/no | Docs? | no <!-- required for new features --> | Issues | Fix #... | License | MIT This is a follow up of symfony/ai#514 with a suggestion of using TranslatableMessage instance for prompts rather than the need in SystemPromptInputProcessor constructor of - A boolean enableTranslation - A domain Since we're passing a TranslatableMessage this also allow to use translation parameters. cc `@OskarStark` that was the suggestion in symfony/ai#514 (comment) Commits ------- 868581f4 [Agent] Allow translatable prompts in `SystemPromptInputProcessor`
… nest `include_tools` (OskarStark) This PR was squashed before being merged into the main branch. Discussion ---------- [AI Bundle] Restructure `system_prompt` configuration to nest `include_tools` | Q | A | |---|---| | Bug fix? | no | | New feature? | yes | | Docs? | yes | | Issues | Eases implementation of [#514](symfony/ai#514) | | License | MIT | ## Summary Restructures `system_prompt` configuration to nest `include_tools` for better organization and ergonomics. ## Changes - Move `include_tools` from agent level to `system_prompt` level - Support both string (simple) and array (advanced) formats ## Before ```yaml system_prompt: 'You are helpful.' include_tools: true ``` ## After ### simple ```yaml system_prompt: 'You are helpful.' ``` ### advanced ```yaml system_prompt: prompt: 'You are helpful.' include_tools: true ``` Commits ------- 5f786809 [AI Bundle] Restructure `system_prompt` configuration to nest `include_tools`
…Processor` (VincentLanglet) This PR was squashed before being merged into the main branch. Discussion ---------- [Agent] Allow translatable prompts in `SystemPromptInputProcessor` | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes/no | Docs? | no <!-- required for new features --> | Issues | Fix #... | License | MIT This is a follow up of symfony/ai#514 with a suggestion of using TranslatableMessage instance for prompts rather than the need in SystemPromptInputProcessor constructor of - A boolean enableTranslation - A domain Since we're passing a TranslatableMessage this also allow to use translation parameters. cc `@OskarStark` that was the suggestion in symfony/ai#514 (comment) Commits ------- 868581f4 [Agent] Allow translatable prompts in `SystemPromptInputProcessor`
So that system prompts can be written in any locale
This adds an optional translator to the
SystemInputProcessor
which, if set, will translate the configured system prompt.