-
-
Notifications
You must be signed in to change notification settings - Fork 285
[AI Bundle][Platform] Make the structured output validation groups configurable #2527
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,10 +11,13 @@ | |
|
|
||
| namespace Symfony\AI\Platform\StructuredOutput\Validator; | ||
|
|
||
| use Symfony\AI\Platform\Event\InvocationEvent; | ||
| use Symfony\AI\Platform\Event\ResultEvent; | ||
| use Symfony\AI\Platform\Exception\InvalidArgumentException; | ||
| use Symfony\AI\Platform\Result\DeferredResult; | ||
| use Symfony\AI\Platform\StructuredOutput\PlatformSubscriber; | ||
| use Symfony\Component\EventDispatcher\EventSubscriberInterface; | ||
| use Symfony\Component\Validator\Constraints\GroupSequence; | ||
| use Symfony\Component\Validator\Validation; | ||
| use Symfony\Component\Validator\Validator\ValidatorInterface; | ||
|
|
||
|
|
@@ -23,21 +26,53 @@ | |
| */ | ||
| final class ValidatorSubscriber implements EventSubscriberInterface | ||
| { | ||
| public const VALIDATION_GROUPS = 'validation_groups'; | ||
|
|
||
| private readonly ValidatorInterface $validator; | ||
|
|
||
| /** @var string|GroupSequence|array<string|GroupSequence>|null */ | ||
| private string|GroupSequence|array|null $invocationGroups = null; | ||
|
|
||
| /** | ||
| * @param string|GroupSequence|array<string|GroupSequence>|null $groups The validation groups to validate the structured output in unless the "validation_groups" option is passed, or null for the validator's default group | ||
| */ | ||
| public function __construct( | ||
| ?ValidatorInterface $validator = null, | ||
| private readonly string|GroupSequence|array|null $groups = null, | ||
| ) { | ||
| $this->validator = $validator ?? Validation::createValidatorBuilder()->enableAttributeMapping()->getValidator(); | ||
| } | ||
|
|
||
| public static function getSubscribedEvents(): array | ||
| { | ||
| return [ | ||
| InvocationEvent::class => 'processInput', | ||
| ResultEvent::class => ['processResult', -10], | ||
| ]; | ||
| } | ||
|
|
||
| public function processInput(InvocationEvent $event): void | ||
| { | ||
| $options = $event->getOptions(); | ||
| $this->invocationGroups = null; | ||
|
|
||
| if (!\array_key_exists(self::VALIDATION_GROUPS, $options)) { | ||
| return; | ||
| } | ||
|
|
||
| $groups = $options[self::VALIDATION_GROUPS]; | ||
|
|
||
| if (null !== $groups && !\is_string($groups) && !\is_array($groups) && !$groups instanceof GroupSequence) { | ||
| throw new InvalidArgumentException('The "validation_groups" option must be a string, an array or a GroupSequence.'); | ||
| } | ||
|
|
||
| $this->invocationGroups = $groups; | ||
|
|
||
| // Consume the option, so it is not forwarded to the provider | ||
| unset($options[self::VALIDATION_GROUPS]); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @chr-hertel question about this mechanic in general: I wonder if unsetting to not forward them is not going to bite in long term. I could imagine being able to properly distinguish between options & provider parameters would be cleaner. What do you think about introducing typed option keys, or dividing the array in 2 keys (e.g. a separate "provider" key in the array which contains all options that should be forwarded to the provider) at a later stage? Just a nitpick but interested in your opinion:) I can imagine this does bring additional complexity because each provider has its own options, but I think it should be doable at one point.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I def see where you're coming from, and that's always a bit painful with those array-wildcards or metadata bags, that buy us flexibility in abstractions. I think for now I'd prefer to be lazy on that and we can re-evaluate when it bites us - but "good instinct" like Claude would write :D |
||
| $event->setOptions($options); | ||
| } | ||
|
|
||
| public function processResult(ResultEvent $event): void | ||
| { | ||
| $options = $event->getOptions(); | ||
|
|
@@ -50,6 +85,7 @@ public function processResult(ResultEvent $event): void | |
| $converter = new ValidatorResultConverter( | ||
| $deferred->getResultConverter(), | ||
| $this->validator, | ||
| $this->invocationGroups ?? $this->groups, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
(This is the same split |
||
| ); | ||
|
|
||
| $event->setDeferredResult(new DeferredResult($converter, $deferred->getRawResult(), $options)); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| <?php | ||
|
|
||
| /* | ||
| * This file is part of the Symfony package. | ||
| * | ||
| * (c) Fabien Potencier <fabien@symfony.com> | ||
| * | ||
| * For the full copyright and license information, please view the LICENSE | ||
| * file that was distributed with this source code. | ||
| */ | ||
|
|
||
| namespace Symfony\AI\Platform\Tests\Fixtures\StructuredOutput; | ||
|
|
||
| use Symfony\Component\Validator\Constraints as Assert; | ||
|
|
||
| final class UserWithGroupedConstraints | ||
| { | ||
| #[Assert\Positive] | ||
| public int $id = 0; | ||
| #[Assert\NotBlank(groups: ['strict'])] | ||
| public string $name = ''; | ||
| } |
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.
Not very happy with this union type.. WDYT?
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.
fine by me - it's honest: it's overloading. we take a bullet for DX