Adds support for Provider HTTP Handling #47
Conversation
There was a problem hiding this comment.
@JasonTheAdams Overall this looks like a great start - it gives me really clean software vibes which I love 😆
A few points of feedback on the interfaces and DTOs mostly. The high level approach looks awesome.
There was a problem hiding this comment.
@JasonTheAdams Overall this looks quite solid. A few more points of feedback.
src/Providers/Http/Contracts/RequestAuthenticationInterface.php
Outdated
Show resolved
Hide resolved
src/Providers/Http/Contracts/RequestAuthenticationInterface.php
Outdated
Show resolved
Hide resolved
src/Providers/Models/Contracts/WithHttpTransporterInterface.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
One additional caveat here regarding header handling.
Either we simply transform all header names always to lowercase, or we define some approach of merging while trying to maintain the original casing, e.g.:
- Keep header name in its original casing.
- For lookup of whether there's a conflict, we always check in a case-insensitive way (we already have the lowercase lookup map for that).
- If there's an attempt to set another header that case-insensitive would have the same name as an existing header, set its values under the new header name and remove the old header name.
It might not be a bad idea to have a single-purpose Headers class that handles all this case-insensitive while maintaining original case lookup and setting in a central place. Not a must have, just an idea.
docs/ARCHITECTURE.md
Outdated
| SomeProviderTextGenerationModel --> HttpTransporterInterface : uses | ||
| SomeProviderTextGenerationModel --> Request : creates | ||
| SomeProviderTextGenerationModel --> Response : receives | ||
| ``` |
There was a problem hiding this comment.
There is a lot of very detailed documentation in here. Maybe that's helpful, but I'm a bit wary of this getting out of date because of how precise and verbose it is. Do we really want to keep all of this in here? If so, have you verified it's up to date with the latest changes you've been making to the actual code in this PR?
There was a problem hiding this comment.
Oops! I don't know why it did that. Good catch! Resolved in 577327a
There was a problem hiding this comment.
@JasonTheAdams This comment was not about the embeddings :)
There was a problem hiding this comment.
My bad. I mixed it up with the other comment.
Honestly, I'm not married to the extra docs at all. I wasn't sure if having it would help Extenders better understand how to use the HttpTransporter and DTOs. But you're right that it's more to maintain, and I'm not convinced in its value. I'm happy to remove this if you think it's best.
There was a problem hiding this comment.
I would say the initial sections, including the Mermaid sequence diagram, are definitely worth keeping. IMO mainly the additional very detailed class diagrams could be removed, because:
- they would mean more substantial additional maintenance keeping it up to date
- they have some overlap with the detailed class diagrams above in the file that are already in place
There was a problem hiding this comment.
@JasonTheAdams This looks really close. A few last points of feedback, most importantly I think we should treat the body (string) as source of truth for the Request. The $data constructor parameter and class property are fully interoperable with it and purely provide an easy layer to set and get raw data.
There was a problem hiding this comment.
@JasonTheAdams Two last follow up comments that I think should be addressed. But they should be quick.
I'm going to tentatively approve, and feel free to merge without another review after you've addressed them. If you have any concerns, let me know. Awesome work, I'm really happy to see this final piece of the foundation land. 🍻
Co-authored-by: Felix Arntz <[email protected]>
Waiting on: #46
This introduces an HTTP system to the Providers framework so models can send requests to the Provider API.
This is built to not depend on a specific HTTP Client (e.g. Guzzle), but instead is happy to work with any PSR-18 compatible Client. It also uses the HTTPlug set of packages to improve auto-discovery of said client.
When we go to implement this into the WordPress system, this will allow us to make our own client that uses (and navigate the quirks of) the WP request functions.