[Agent][Platform] Add support for native union types and list of polymporphic* types by using DiscriminatorMap
#585
Conversation
HaKIMus
requested review from
Nyholm,
OskarStark and
chr-hertel
as code owners
carsonbot
changed the title
OskarStark
added
Platform
carsonbot
changed the title
OskarStark
changed the title
DiscriminatorMap
|
Maybe I miss something, but why do we need the public function __invoke(
Foo|Bar $myVar,
) {}Doesn't this contain enough information to resolve it? |
It might be, but the problematic case is a If Symfony Serializer would understand how to work with annotated |
HaKIMus
changed the title
DiscriminatorMapDiscriminatorMap
|
I have one question, can we simplify the fixture classes to a bare minimum and use simple terminology? I now this is from your real world use case, but for tests, maintenance too much complexity for me. if we need the enum inside the union type, fine, if not, lets remove it or lets only keep 2 values etc. Thanks |
|
@OskarStark I'll simplify and try to make the new DTOs examples more comprehensible max 'till Tomorrow. @chr-hertel when you ask for examples - you mean like the ones in PR description or something like |
|
lets add |
fixtures/StructuredOutput/PolymorphicType/ListItemDiscriminator.php
Outdated
Show resolved
Hide resolved
| { | ||
| public function __construct( | ||
| public string $name, | ||
| #[With(pattern: '^name$')] public string $type = 'name', |
There was a problem hiding this comment.
| #[With(pattern: '^name$')] public string $type = 'name', | |
| #[With(pattern: '^name$')] | |
| public string $type = 'name', |
|
I've simplified the examples to simpler objects and organized them in separated namespaces. I'm not sure why the pipeline fails with: Locally the test runs correctly, and I'm on version 8.4. None of us wants to introduce a ghost error the project, it'd be good to know why it has happened, and I, at the moment, have no clue. UPDATE |
|
@OskarStark @chr-hertel in the release 8.0 of the symfony/serializer package DiscriminatorMap doesn't implement I'm thinking of the following solutions:
Wdyt? 7.3: https://github.com/symfony/serializer/blob/7.3/Attribute/DiscriminatorMap.php EDIT |
src/agent/CHANGELOG.md
Outdated
| * Add support for polymorphic types via DiscriminatorMap | ||
| * Add support for union types |
There was a problem hiding this comment.
| * Add support for polymorphic types via DiscriminatorMap | |
| * Add support for union types | |
| * Add support for union types and polymorphic types via DiscriminatorMap |
|
One minor and good to merge |
…mporphic* types by using `DiscriminatorMap`
OskarStark
force-pushed
the
feature/union-type-support
branch
from
0ed5448 to
36f5bb4
Compare
|
Thanks for your work on this new feature! |
|
@HaKIMus does the example |
|
I used the When I set the name explicitly as 'gpt-5-nano', I started to get errors about unknown named parameter logger. To me it looks like an error with autoloading or my local set up (I didn't run docker compose to save resources on my machine), as the constant name and the logger param definitely exist. I must say I never run the example, I've assumed the "compiling" correctness of the |
|
found this issue on |
|
From what I've deduct - we're not setting nullable properties in the required list, but all structure keys must be present in it. Because the $time property is accepting a null the field is not required. If we remove the null as an option - it's present in the required list. The behavior is strange, however, because examples like I've actually tested the cases myself, and that's true. To summarize Nullable fields must be present in the required list of their parent output structure, otherwise platforms (like openai) will reject the call. Fix The fix could be as follow: if ($type->isNullable()) {
// anyOf already contains the null variant when applicable; do nothing
if (!isset($schema['anyOf'])) {
$schema['type'] = [$schema['type'], 'null'];
}
}
if (!($element instanceof \ReflectionParameter && $element->isOptional())) {
$result['required'][] = $name;
}
We've assumed the nullable fields aren't required, and the fix separates the logic of setting an element as required from the fact if it's null or not. Before the fix: After the fix: Namespaces are different, because it was easier for me to run the example in my project. |
|
@chr-hertel I was about to open a PR, but I see you've this one open: #802. Do you want me to implement the feature of treating nullables as required or you're going to handle it? |
|
@HaKIMus no no, feel free to go ahead - didn't address that issue and didn't change the schema part |
|
Additionally, I'd like to clarify the discordance, I wrote:
I've just checked, and none of my properties of the 7 possible structured outputs are nullable, and that's why - aside from not running the example - I've never stumbled upon the issue. I apologize for my mistake. |
This PR was squashed before being merged into the main branch. Discussion ---------- Feature/treat nullable fields as required | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes <!-- please update src/**/CHANGELOG.md files --> | Docs? | no <!-- required for new features --> | Issues | Fix #585 <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT Context: #585 (comment) Summary: Nullable fields must be present in the required list of their parent output structure, otherwise platforms (like openai) will reject the call. Proof of work: #585 (comment) `structured-output-union-types.php` output: ```php file:///~/ai/examples/openai/structured-output-union-types.php#L33 Symfony\AI\Fixtures\StructuredOutput\UnionType\UnionTypeDtofile:///~/ai/fixtures/StructuredOutput/UnionType/UnionTypeDto.php#L14 {#310 +time: Symfony\AI\Fixtures\StructuredOutput\UnionType\HumanReadableTimeUnionfile:///~/ai/fixtures/StructuredOutput/UnionType/HumanReadableTimeUnion.php#L14 {#316 +readableTime: "2023-10-07T19:32:10Z" } } ``` <!-- Replace this notice by a description of your feature/bugfix. This will help reviewers and should be a good start for the documentation. Additionally (see https://symfony.com/releases): - Always add tests and ensure they pass. - For new features, provide some code snippets to help understand usage. - Features and deprecations must be submitted against branch main. - Update/add documentation as required (we can help!) - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry - Never break backward compatibility (see https://symfony.com/bc). --> Commits ------- 57d9b91 Feature/treat nullable fields as required
4 participants
It's a draft, because, primarily, I want to ask if the DiscriminatorMap approach is ok with you.
Why the discriminator map usage?
I've achieved the native
list<Foo|Bar>union type support for agent json schema output, but Symfony Serializer wasn't working with it correctly (list<TableDataVisualizationDto|ChartDataVisualizationDto>), outputting an array in\Symfony\Component\Serializer\SerializerInterface::deserialize::112. If you know how to make the serializer work with the "nested" union types correctly, then let me know and we might achieve a native support without the DiscriminatorMap usage.Example
This is an example from my project I want the feature to work on.
Model I tried it with:
gpt-5-nanowith theSymfony\AI\Platform\Bridge\OpenAi\Gptmodel class.Prompt:
Schema produced:
Schema dump
The description is long enough, so I'd skip the PHP DTOs structure. They look almost identical to the ones I used in tests in the PR.
Output (the data are fake):
Output dump