Consolidate API request logic with HTTPClient abstraction #988
Conversation
| $response_message = wp_remote_retrieve_response_message( $response ); | ||
| $status_text = $response_message ? $response_message : __( 'Unknown error', 'classifai' ); | ||
|
|
||
| // Try to extract specific error message from the server response |
There was a problem hiding this comment.
Former code had chance for improvement:
$message = $json['error']['message'] ?? esc_html__( 'An error occurred', 'classifai' );It should be checking on $json['error']['message'] only if $json['error'] isn't a string itself.
It's expected now for users to see much more details from providers on errors instead of former An error ocurred message only.
|
|
||
| use Classifai\Providers\Provider; | ||
| use Classifai\Providers\OpenAI\APIRequest; | ||
| use Classifai\Providers\Localhost\APIRequest; |
There was a problem hiding this comment.
No longer using OpenAI for local providers, now they have its own clients
| if ( null === self::$scheduler_instance ) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This was throwing a fatal error when key was empty / with error.
| $this->username = get_username(); | ||
| $this->password = get_password(); |
There was a problem hiding this comment.
Kept specificities from Watson auth
MiguelAxcar
changed the title
|
Thanks for the PR @MiguelAxcar. I noticed we already have a similar ongoing PR (#994), which is doing the same thing along with integrating the php-ai-client. It’s currently implemented only for OpenAI, with plans to extend it to other providers as well. @dkotter, since you’re the author of that PR, I’d like your feedback on the potential next steps for this one. Whether we should update #994 to extend the base class for all providers (I’d prefer to keep it small initially and focus on php-ai-client or wp-ai-client integration, then handle other providers in a follow-up), tweak this PR to make it useful for #994, or keep it on hold until the ongoing discussions in #994 are finalized. Thanks! |
Yes, thanks for the work here @MiguelAxcar and I did take a look at this to see if there was anything I should be implementing with the work I was doing in #994. That work is still early stage, as the PHP AI Client (and now the WP AI Client) are still in early development, but we're likely to take at the very least a similar approach that is in that PR. For now, I'd like to leave this PR open but we're likely to consolidate our approaches here once the AI Client is more stable. I'll come back to this PR at that point and decide next best steps. |
|
Thanks @dkotter @iamdharmesh for the feedback. My intent here is to first land a provider-agnostic HTTP layer (auth/headers/errors/timeouts/specificities) and migrate providers onto it, while As mentioned by Dharmesh #994 experiment bridges only OpenAI; great exploration of course, awesome to see this in action first hand btw, but potentially never shipped or will need changes to work. Given that, I would suggest keeping this (or another) PR focused on consolidating the HTTP client across providers, then introduce the PHP AI Client bridge as a small, targeted follow-up, using results from #994 experiment / exploration, That order keeps the blast radius small, preserves parity, and makes the Happy to help split/cherry-pick pieces to get this in smoothly, or any help I could provide for this. |
Description of the Change
This PR intends to extract common HTTP functionality into a new
HTTPClientbase class.Reasons
Integrating https://github.com/WordPress/php-ai-client may be a good idea for the future as it would give ClassifAI a standardized way to communicate with AI providers, reduce the amount of custom request code maintained, and align with a broader WP ecosys standard.
My idea / proposal is to land this refactor first to centralize the shared HTTP code in
HTTPClient, handling potential adjustments, then wait for php-ai-client to have a release then add a small bridge with minimal changes to integrate it.How to test the Change
Changelog
Credits
Props @MiguelAxcar
Checklist: