-
-
Notifications
You must be signed in to change notification settings - Fork 48
[Platform] Use JSON Path to convert responses #136
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
ed6eaea
to
7ec4f22
Compare
I really like the idea! Know that if you manage to get a resource for the response, JsonPath uses JsonStreamer to optimize the part to actually decode. Just in case 🙂 Given that JsonPath is experimental, I'd suggest changing the composer constraint to 7.3.*. That's what we do in core because experimental components may not be compatible between their minor versions. I have no plan to change classes or signatures in the JsonPath component until it's stable release as I managed to make it RFC compliant lately with the exact same API. But better be safe than sorry! |
Interesting approach! Do we even need the platform-specific EDIT: ok hmm I guess the idea was to encapsulate the "protocol" in the converter to be reused in different platforms |
8b26c09
to
4387b79
Compare
Thanks for that!
exactly |
4387b79
to
b820262
Compare
We just created some Thanks |
a5e2e28
to
b4ef6f5
Compare
eee446f
to
a5d834a
Compare
src/platform/src/Contract/JsonPathConverter/ResultExtractorInterface.php
Outdated
Show resolved
Hide resolved
2d2c547
to
459b7a3
Compare
459b7a3
to
afe308c
Compare
d927cce
to
abec2aa
Compare
9aedbf3
to
9a76a18
Compare
34e7852
to
eff76f7
Compare
750ab79
to
61548ce
Compare
435d8f6
to
6a7b602
Compare
Running CS fixer locally doesn't help? |
5db093a
to
3affdb8
Compare
…(chr-hertel) This PR was merged into the main branch. Discussion ---------- [Platform] Simplify choice handling with multiple results | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | Docs? | no | Issues | | License | MIT The idea of choices is that a completion result could have multiple results when option `n` larger than `1`. Until now the `Choice` class was rather simple and just assumed text or tool - and therefore in theory other choices where impossible. not sure anyone ever had an issue with this, but this now is more correct with encapsulating multiple results in the `ChoiceResult`. Makes the code even easier and more correct. Pulled out of #136 to reduce the noise. Examples: <img width="1507" height="1059" alt="image" src="https://github.com/user-attachments/assets/a151dfee-ccdf-4cae-9a94-327cf57124d3" /> Commits ------- 3affdb8 Simplify choice handling
6a7b602
to
60e1fec
Compare
cca319e
to
af6657d
Compare
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.
Pull Request Overview
This PR introduces a JSON Path-based approach for converting AI platform responses, aiming to centralize response conversion logic and reduce duplication across different AI provider implementations. The implementation combines an OpenAI-based central converter with configurable JSON Path extraction mechanisms for different data types.
- Establishes a unified response conversion system using JSON Path for data extraction
- Adds comprehensive test fixtures for various AI platform response formats (Gemini, OpenAI, etc.)
- Introduces fixtures for different response types including completions, embeddings, tool calls, and streaming responses
Reviewed Changes
Copilot reviewed 64 out of 64 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/platform/tests/Contract/fixtures/embeddings-gemini.json | Test fixture for Gemini embedding response format |
src/platform/tests/Contract/fixtures/embeddings-default.json | Test fixture for default (OpenAI) embedding response format |
src/platform/tests/Contract/fixtures/completions-gemini-toolcall.json | Test fixture for Gemini tool call completion response |
src/platform/tests/Contract/fixtures/completions-gemini-text.json | Test fixture for Gemini text completion response |
src/platform/tests/Contract/fixtures/completions-gemini-stream | Test fixture for Gemini streaming completion responses |
src/platform/tests/Contract/fixtures/completions-gemini-error.json | Test fixture for Gemini error response format |
src/platform/tests/Contract/fixtures/completions-gemini-choices.json | Test fixture for Gemini completion with multiple choice responses |
src/platform/tests/Contract/fixtures/completions-default-toolcall.json | Test fixture for default (OpenAI) tool call completion response |
src/platform/tests/Contract/fixtures/completions-default-toolcall-multiple.json | Test fixture for default completion with multiple tool calls |
src/platform/tests/Contract/fixtures/completions-default-toolcall-choices.json | Test fixture for default completion with tool call choices |
src/platform/tests/Contract/fixtures/completions-default-text.json | Test fixture for default text completion response |
src/platform/tests/Contract/fixtures/completions-default-stream-toolcall | Test fixture for default streaming tool call completion |
af6657d
to
0a97792
Compare
we need a centralized result conversion heavily-inspired by the OpenAI contract - but we don't have to use JSON Path here. what do you think? clever thing to do or too artistic? @OskarStark @DZunke @alexandre-daubois @lyrixx @derrabus @xabbuh @fabpot @valtzu we end up having customized result conversion with extraction paths like this: ai/src/platform/src/Bridge/Anthropic/PlatformFactory.php Lines 39 to 52 in 0a97792
|
This approach combines two ideas:
And of course those two things don't have to be coupled, but one thing led to the other somehow ...