-
-
Notifications
You must be signed in to change notification settings - Fork 48
[AIBundle] Improve injection aliases #281
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
base: main
Are you sure you want to change the base?
Conversation
7a73288
to
0b218be
Compare
], | ||
]); | ||
|
||
$this->assertTrue($container->hasAlias('.Symfony\AI\Agent\AgentInterface $my_agentAgent')); |
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.
Shouldn't it be $myAgentAgent?
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.
Well, I'm not sure, while adding the call, the container export the service definition as the following:
<service id="Symfony\AI\Agent\AgentInterface $fooAgent" alias="ai.agent.foo"/>
I bet Symfony is exporting the service along with the FQCN, don't know why to be fair but looks like the "default behavior" 🤔
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.
Your name is my_agent
, so IMHO it should be myAgent
+ Agent
suffix, to be more clear, I would expect $fooBarAgent
when I name it foo_bar
and not $foo_barAgent
. How does Monolog is converting the names for example or HttpClient?
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.
Well, regarding the method:
/**
* Registers an autowiring alias that only binds to a specific argument name.
*
* The argument name is derived from $name if provided (from $id otherwise)
* using camel case: "foo.bar" or "foo_bar" creates an alias bound to
* "$fooBar"-named arguments with $type as type-hint. Such arguments will
* receive the service $id when autowiring is used.
*/
public function registerAliasForArgument(string $id, string $type, ?string $name = null): Alias
{
$parsedName = (new Target($name ??= $id))->getParsedName();
if (!preg_match('/^[a-zA-Z_\x7f-\xff]/', $parsedName)) {
if ($id !== $name) {
$id = \sprintf(' for service "%s"', $id);
}
throw new InvalidArgumentException(\sprintf('Invalid argument name "%s"'.$id.': the first character must be a letter.', $name));
}
if ($parsedName !== $name) {
$this->setAlias('.'.$type.' $'.$name, $type.' $'.$parsedName);
}
return $this->setAlias($type.' $'.$parsedName, $id);
}
It look like Sf is not doing the job properly, here's the method usage for HttpClient's:
$container->registerAliasForArgument($name, HttpClientInterface::class);
if ($hasPsr18) {
$container->setDefinition('psr18.'.$name, new ChildDefinition('psr18.http_client'))
->replaceArgument(0, new Reference($name));
$container->registerAliasForArgument('psr18.'.$name, ClientInterface::class, $name);
}
if ($hasHttplug) {
$container->setDefinition('httplug.'.$name, new ChildDefinition('httplug.http_client'))
->replaceArgument(0, new Reference($name));
$container->registerAliasForArgument('httplug.'.$name, HttpAsyncClient::class, $name);
}
BTW, If I try to define a custom agent in my application called translation
, the resolved argument is translationAgent
.
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.
Regarding Monolog, there's nothing more in the extension:
protected function createLogger(string $channel, string $loggerId, ContainerBuilder $container)
{
if (!in_array($channel, $this->channels)) {
$logger = new ChildDefinition('monolog.logger_prototype');
$logger->replaceArgument(0, $channel);
$container->setDefinition($loggerId, $logger);
$this->channels[] = $channel;
}
$parameterName = $channel . 'Logger';
$container->registerAliasForArgument($loggerId, LoggerInterface::class, $parameterName);
}
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.
Ok, found the issue.
Looks like if you set foo_toto
, Symfony resolve it as fooToto
, the catch is that if the $name
is not the same as the parsed name:
if ($parsedName !== $name) {
$this->setAlias('.'.$type.' $'.$name, $type.' $'.$parsedName);
}
In this case, the $name
is used (so foo_toto
) rather than fooToto
(the parsed one) 🤔
EDIT:
One solution might be to call the Target
class while defining the alias:
$container->registerAliasForArgument('ai.agent.'.$name, AgentInterface::class, (new Target($name.'Agent'))->getParsedName());
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.
Thanks for digging, I am fine either way, but would like to end with $myAgentAgent
for my_agent
😄
Maybe something which can be unified? cc @nicolas-grekas
Hi 👋🏻
This PR aims to add "injection aliases", those allows the user to use the following code while defining X agents:
Why the
Agent
suffix? Because we don't know, maybe one day, a service will be calledfoo
and it will trigger an error, it seems better to define a suffix and ensure that this bundle is the only one to use it.