-
Notifications
You must be signed in to change notification settings - Fork 15
Add the Title Generation execute functionality #67
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
Add the Title Generation execute functionality #67
Conversation
…ategory for each ability
…he AI Client to actually make the requests
… easily set credentials. Initialize the settings screen
… this into our request method
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #67 +/- ##
=============================================
+ Coverage 48.48% 60.94% +12.45%
- Complexity 45 81 +36
=============================================
Files 6 10 +4
Lines 198 425 +227
=============================================
+ Hits 96 259 +163
- Misses 102 166 +64
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ge. Load this file as our prompt if it exists
…ert that to a string when sending it in a request
…using newer models that may not be fully supported
…assing the feature into the ability. Move the get_post_context method into a helper function so other functionality can take advantage of this. Move the system instructions into the ability
…ts those instructions to account for this
…nt now. Remove line changes in test files that weren't intended
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @matysanchez, @prabinjha, @Meenakshi-bose. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
JasonTheAdams
left a comment
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.
Excited to see the first usage of the WP AI Client, @dkotter!
I didn't fully review this, but I got confused enough by the architectural direction to pause here for some questions. I feel like this is over abstracted, and if it's not I'm really curious about the needs if not.
includes/API_Request.php
Outdated
| /** | ||
| * Check if the AI SDK Client is available. | ||
| * | ||
| * @since 0.1.0 | ||
| * | ||
| * @return bool True if the client is available, false otherwise. | ||
| */ | ||
| protected function is_client_available(): bool { | ||
| return class_exists( AI_Client::class ); | ||
| } |
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.
Why wouldn't this be available?
includes/API_Request.php
Outdated
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.
I'm really confused by this class. Why do we need this? Why not just do AI_Client::prompt() in the Title_Generation Ability and do everything we need there? Maybe I'm missing something, but the whole abstraction of this class seems unnecessary to me.
I think the biggest thing it's doing is applying the model and provider, if specified. If that's all it's doing, then I'd make a function (or class) somewhere that just creates the Prompt_Builder, applies the Provider/Model preferences, and then returns the Prompt_Builder instance. That really leans into the point of the builder concept, where you can hand over a partially configured builder intsance.
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.
So I debated on this so happy to simplify. Few thoughts here:
Was trying to reduce the amount of code we would need to duplicate as we add functionality in the future that needs to make requests. Meaning for generating titles, beyond just passing in a prompt, we set the system instructions, we set the temperature value, we set the candidate count value and we set the default model preferences. I could easily move that out of this class and into the Title Generation Ability but we would then have that exact (or similar) code duplicated each time.
This class also standardizes how we process the response, ensuring we return an error if we get an empty array back and sanitizing the responses before returning. Again, all things that could happen in the Ability but would require us to duplicate that code each time.
That said, I'm fine with the approach of a helper function or two that will build the Prompt_Builder and return it and then each Ability/Feature/Experiment/Whatever that wants to make requests can take over from there. Let me make those changes and see if that feels like a better approach
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.
I see where you're coming from. My main motivation here is that we want to think of these Abilities as examples, and I want devs to see how easy it is to construct a prompt and get the results in the ability.
If we need to normalize the response for our abilities in some way, then I'd like to do that after using the builder. That will also help us see if there's some sort of sanitization/normalization that we should introduce in the WP AI Client in some way.
Just to make sure you're aware, there's also a Prompt_Builder_With_WP_Error that can be used if you want to avoid exceptions.
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.
So I've simplified this a bit in ca74ff1, getting rid of the API_Request class and adding a helper function that builds our prompt builder instead.
If that still feels too abstracted (which I've gone back and forth on and can't decide), we can remove all of that and instead, just do this directly in the generate_titles method:
$prompt_builder = AI_Client::prompt_with_wp_error( '"""' . $context . '"""' );
$prompt_builder = $prompt_builder->using_system_instruction( $this->get_system_instruction() );
$prompt_builder = $prompt_builder->using_temperature( 0.7 );
$prompt_builder = $prompt_builder->using_candidate_count( (int) $candidates );
$prompt_builder = $prompt_builder->using_model_preference( ...get_preferred_models() );That's obviously cleaner and a lot less code but does mean we would have very similar (if not exact) code for each experiment we add. But as you've said, would be great if people can look at the code here and easily understand how things work, even if that means we end up with some code duplication
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.
Getting there! A couple notes:
- If you specify a model you don't need to specify a provider, as a model implies a provider
- You probably don't need to specify the preferred models array. The Prompt Builder will simply select a model that fits the prompt. Preferring a model is for when you have something like Nano Banana setup and you want to prefer that for image generation.
Also, chaining can be cleaned up a lot (without re-assigning the variable repeatedly):
$prompt_builder = AI_Client::prompt_with_wp_error( '"""' . $context . '"""' );
->using_system_instruction( $this->get_system_instruction() );
->using_temperature( 0.7 );
->using_candidate_count( (int) $candidates );
->using_model_preference( ...get_preferred_models() );I don't think the helper is really needed, as I'm not really seeing any code duplication that's necessary.
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.
Okay, more simplification done in 94dff32
You probably don't need to specify the preferred models array
The reason I added this is because I ran into this issue when testing: WordPress/php-ai-client#117. Basically things worked great one day and the next day things started to fail. In debugging, found out it was because the Prompt Builder started using the gpt-4o-mini-search-preview model which doesn't work properly for text generation.
So I can remove this but feels safer to have some defaults to avoid issues where things might break from one day to the next if new models are released or the models API results change.
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.
Ohhh, that's good for us to know. I'm going to ping @felixarntz on this one and get his thoughts. That sounds like a potential bug for that model's capabilities in PHP AI Client.
Definitely let us know if you run into stuff like that. You're the first one bringing this all together, so there are quite likely bugs like this. 😆
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.
I replied in WordPress/php-ai-client#117 (comment):
- Concrete problem: Our OpenAI model capability and option assignments are probably inaccurate / out of date.
- Underlying problem: We should not return the alphabetically first eligible model, but rather a flagship model of the provider that makes sense to prefer by default.
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.
I'd say for now let's put a preference list here, and then remove it once we've fixed the upstream bug.
…ions. Use this new helper function to get a prompt builder and then use that to make our requests
…uilder into the ability execution
JasonTheAdams
left a comment
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.
Finished going through it! Left a few minor suggestions, but it's really close!
| /** | ||
| * TODO: Might be interesting to add simple Abilities for the following, | ||
| * just as a way to demonstrate a different approach to registering Abilities, | ||
| * how to call Abilities via PHP and how multiple Abilities can be used together. | ||
| * | ||
| * Example: Get post content Ability; get post author Ability; get post terms Ability. | ||
| */ |
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.
Agreed! I like the idea of composing abilities!
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.
I'll open another PR after this merges that adds these in and we can continue conversation on that
| * Automatic detection order: | ||
| * 1. `system-instruction.php` | ||
| * 2. `prompt.php` |
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.
I'm curious, why have more than one option at this time? Why not just stick with one to stick to a single convention?
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.
Good catch. Originally had this named as prompt and then renamed to match what the Prompt Builder expects and left support for both. But as this is a net-new thing, no need to support both right now. Removed in 8619a72
| } | ||
| } | ||
|
|
||
| return ''; |
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.
Do we want silent failure here? Maybe yes, just double-checking. 🙂
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.
That was the thought, yes, as the Prompt Builder handles that fine. Could change this to return an error that we output, though that would require us to not have the same chaining we have right now on the Prompt Builder as we'd need to check if an error was returned before calling using_system_instruction
JasonTheAdams
left a comment
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.
Great work, @dkotter! LGTM! 🎉
| return AI_Client::prompt_with_wp_error( '"""' . $context . '"""' ) | ||
| ->using_system_instruction( $this->get_system_instruction() ) | ||
| ->using_temperature( 0.7 ) | ||
| ->using_candidate_count( (int) $candidates ) | ||
| ->using_model_preference( ...get_preferred_models() ) | ||
| ->generate_texts(); |
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.
One note I'll make here that I don't think we need to do anything about at this time, is the use of the Prompt_Builder::is_supporteed_for_text_generation() method. This is intended to make it clear if there is not a model that supports this prompt's needs, allowing us to do something in that condition.
I want to add a WP_Ability::is_available() method in the future for checking ahead of time if an Ability will contextually work. I think this check would fit well there.
What?
Partially closes #10
Builds off of the Title Generation feature base work done in #61 to wire up the actual execution of title generation. Note the actual UI still isn't in this PR, there will be a close followup that adds that.
Why?
In PR #61, we introduced the Title Generation Ability and Feature classes but the actual generation of titles wasn't completed there. This PR finishes that piece, though note there still isn't any UI to trigger this functionality, it all needs to be done via direct API requests.
How?
trunkbranch of the WP AI Client repo to take advantage of the new settings screen and helper classes introduced there. This can be changed to pull from an actual release once that is outAPI_Requestclass that can be used to make requests to an AI Provider. This ultimately utilizes the WP AI Client. Debated whether having this extra abstraction was worth it so can change this if desired. But it felt like the right approach knowing we'll have multiple other AI integrations coming that will all have slightly different requirements (i.e. arguments) they'll need to sendgenerate_titlesmethod to the Title Generation Ability and uses that in the execute callbackTesting Instructions
There is no UI yet but can be tested by making direct API requests to the Title Generation Ability run endpoint.
First you'll need to login to WordPress and go to
Settings > AI Credentialsand add in at least one API key.Then make an authenticated
POSTrequest to the Title Generation run endpoint and ensure a title is generated based on the content you provideMake an authenticated
POSTrequest to the Title Generation run endpoint, passing in a valid post ID, and ensure a title is generatedMake an authenticated
POSTrequest to the Title Generation run endpoint, passing in a valid post ID and number and ensure multiple titles are generatedMake an authenticated
POSTrequest to the Title Generation run endpoint and don't pass any content or post ID and ensure an error is returnedTest using WordPress Playground
The changes in this pull request can be previewed and tested using this WordPress Playground instance:
Click here to test this pull request.