-
Notifications
You must be signed in to change notification settings - Fork 15
Add the Title Generation feature base #61
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
Changes from 28 commits
4a9c4d7
475a827
dae9ef6
96912e2
82319aa
8b64bc9
5de7754
4f1f1c4
16c2a9c
aa0a956
70069f5
c34f992
6594c5b
5607fce
9aea208
79318d3
dcec2f6
a574ccb
88eb392
e273802
1beac35
87bd000
779d3d6
fff6c5d
550b4fb
032a3e7
381e2b7
86f1ebe
e0e4873
9b9b123
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,172 @@ | ||
| <?php | ||
| /** | ||
| * Title generation WordPress Ability implementation. | ||
| * | ||
| * @package WordPress\AI | ||
| */ | ||
|
|
||
| declare( strict_types=1 ); | ||
|
|
||
| namespace WordPress\AI\Abilities; | ||
|
|
||
| use WP_Error; | ||
| use WordPress\AI\Abstracts\Abstract_Ability; | ||
|
|
||
| /** | ||
| * Title generation WordPress Ability. | ||
| * | ||
| * @since 0.1.0 | ||
| */ | ||
| class Title_Generation extends Abstract_Ability { | ||
|
|
||
| /** | ||
| * Returns the category of the ability. | ||
| * | ||
| * @since 0.1.0 | ||
| * | ||
| * @return string The category of the ability. | ||
| */ | ||
| protected function category(): string { | ||
| return 'ai-experiments'; // TODO: add a reusable way to get the category slug? | ||
| } | ||
|
|
||
| /** | ||
| * Returns the input schema of the ability. | ||
| * | ||
| * @since 0.1.0 | ||
| * | ||
| * @return array<string, mixed> The input schema of the ability. | ||
| */ | ||
| protected function input_schema(): array { | ||
| return array( | ||
| 'type' => 'object', | ||
| 'properties' => array( | ||
| 'content' => array( | ||
| 'type' => 'string', | ||
| 'sanitize_callback' => 'sanitize_text_field', | ||
| 'description' => esc_html__( 'Content to generate title suggestions for.', 'ai' ), | ||
| ), | ||
| 'post_id' => array( | ||
| 'type' => 'integer', | ||
| 'sanitize_callback' => 'absint', | ||
| 'description' => esc_html__( 'Content from this post will be used to generate title suggestions. This overrides the content parameter if both are provided.', 'ai' ), | ||
| ), | ||
| 'n' => array( | ||
| 'type' => 'integer', | ||
| 'minimum' => 1, | ||
| 'maximum' => 10, | ||
| 'sanitize_callback' => 'absint', | ||
| 'description' => esc_html__( 'Number of titles to generate', 'ai' ), | ||
| ), | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the output schema of the ability. | ||
| * | ||
| * @since 0.1.0 | ||
| * | ||
| * @return array<string, mixed> The output schema of the ability. | ||
| */ | ||
| protected function output_schema(): array { | ||
| return array( | ||
| 'type' => 'object', | ||
| 'properties' => array( | ||
| 'titles' => array( | ||
| 'type' => 'array', | ||
| 'description' => esc_html__( 'Generated title suggestions.', 'ai' ), | ||
| 'items' => array( | ||
| 'type' => 'string', | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Executes the ability with the given input arguments. | ||
| * | ||
| * @since 0.1.0 | ||
| * | ||
| * @param mixed $input The input arguments to the ability. | ||
| * @return mixed|\WP_Error The result of the ability execution, or a WP_Error on failure. | ||
| */ | ||
| protected function execute_callback( $input ) { | ||
| // Default arguments. | ||
| $args = wp_parse_args( | ||
| $input, | ||
| array( | ||
| 'content' => null, | ||
| 'post_id' => null, | ||
| 'n' => 1, | ||
| ), | ||
| ); | ||
|
|
||
| // If a post ID is provided, ensure the post exists before using its' content. | ||
| if ( $args['post_id'] ) { | ||
| $post = get_post( $args['post_id'] ); | ||
|
|
||
| if ( ! $post ) { | ||
| return new WP_Error( | ||
| 'post_not_found', | ||
| /* translators: %d: Post ID. */ | ||
| sprintf( esc_html__( 'Post with ID %d not found.', 'ai' ), absint( $args['post_id'] ) ) | ||
| ); | ||
| } | ||
|
|
||
| $args['content'] = $post->post_content; | ||
| } | ||
|
|
||
| // If we have no content, return an error. | ||
| if ( ! $args['content'] ) { | ||
| return new WP_Error( | ||
| 'content_not_provided', | ||
| esc_html__( 'Content is required to generate title suggestions.', 'ai' ) | ||
| ); | ||
| } | ||
|
|
||
| // TODO: Implement the title generation logic. | ||
|
|
||
| return array( | ||
| 'name' => $this->get_name(), | ||
| 'label' => $this->get_label(), | ||
| 'description' => $this->get_description(), | ||
| 'content' => wp_kses_post( $args['content'] ), | ||
| 'post_id' => absint( $args['post_id'] ) ?? esc_html__( 'Not provided', 'ai' ), | ||
JasonTheAdams marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 'n' => absint( $args['n'] ), | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the permission callback of the ability. | ||
| * | ||
| * @since 0.1.0 | ||
| * | ||
| * @param mixed $args The input arguments to the ability. | ||
| * @return bool|\WP_Error True if the user has permission, WP_Error otherwise. | ||
| */ | ||
| protected function permission_callback( $args ) { | ||
| if ( ! current_user_can( 'edit_posts' ) ) { | ||
JasonTheAdams marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| return new WP_Error( | ||
| 'insufficient_capabilities', | ||
| esc_html__( 'You do not have permission to generate titles.', 'ai' ) | ||
| ); | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the meta of the ability. | ||
| * | ||
| * @since 0.1.0 | ||
| * | ||
| * @return array<string, mixed> The meta of the ability. | ||
| */ | ||
| protected function meta(): array { | ||
| return array( | ||
| 'show_in_rest' => true, | ||
| ); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| <?php | ||
| /** | ||
| * Abstract Ability base class. | ||
| * | ||
| * @package WordPress\AI\Abstracts | ||
| */ | ||
|
|
||
| declare( strict_types=1 ); | ||
|
|
||
| namespace WordPress\AI\Abstracts; | ||
|
|
||
| use WP_Ability; | ||
|
|
||
| /** | ||
| * Base implementation for a WordPress Ability. | ||
| * | ||
| * @since 0.1.0 | ||
| */ | ||
| abstract class Abstract_Ability extends WP_Ability { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm strongly against extending If there's justification for an abstract class, then we should follow the pattern of
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have discussed this many times before: It's fine to extend
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Except we're in the (Which I believe is a more accurate summary of the discussion, most recently iirc WordPress/abilities-api#97 ). So, for our specific plugin / this PR:
The two biggest downsides of extending
(IMO the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If an API allows something, it's fine to do it. It doesn't mean it's encouraged, and it doesn't mean it's discouraged. If the API wants it to be discouraged, it shouldn't even be possible in the first place. This project, like any other project, has to choose one of those two approaches, and which approach is chosen is personal preference. Your personal preference is to not extend To your questions:
Many aspects of coding are subjective. Whether this project uses procedural programming or OOP is subjective, and so is whether it extends
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Weighing in here, I kindly suggest we step back from the "is extending That said, I do think it would be a good idea to also include a procedural example in this project, which we can reference in the documentation as something intentional. That way we can help both procedurally and OOP inclined folks on how they can interact with the Abilities API in their preferred manner. In short, I think this is a great both-and situation where we can show off how flexibly extensible this API is.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lively conversation! Appreciate the passion and continued clarification on both sides! 😄 Thanks for providing the example and further context, @justlevine! It helps! I took some time to reflect and really tried to think through what you're suggesting. I think I've somewhat returned back to my original point, but with some clarification. That is, providing two examples as part of this repo for educational purposes:
This makes me like the way this works even more. The idea of having an In conclusion, my preference is how this currently works because it illustrates
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yup, that's all of it 🙇. Basically the
@JasonTheAdams I'm not entirely sure I understood what you mean here. Can you clarify? At risk of being redundant, we wouldn't be testing or demonstrating IOW this doesn't preclude (If that was already clear apologies, I'm signing out for the night, and didn't want to hold up the convo with another round trip 😅)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
( @felixarntz In fairness one of the main reasons I introduced
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Appreciate all the input, @justlevine, and referencing this as following the At this point, I think we need to merge this PR and can revisit this in a subsequent PR. As I've said, I'm open to having multiple ways of registering abilities to show folks the versatility of registration. This isn't a dismissal, @justlevine, as we've spent a good amount of time on this and the majority of folks contributing to the conversation remain unconvinced, so we ought to move forward. Please consider making a PR to introduce another Feature which we can contrast this with.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @JasonTheAdams , and I agree that iterating quickly is the goal here and happy to kick the discussion to a follow-up PR so we can keep progressing. Correct me if I'm wrong (cc @jeffpaul ), but my gut says that we have at least until 6.9 ships - if not a bit longer - before any architectural decisions or reversing them have any real impact. (Tangentially, we're also overdue a discussion about WPCS, contribution guidelines/barriers to entry for new modules/experiments, and what a path to core for specific experiments/features would look like) |
||
|
|
||
| /** | ||
| * Constructor. | ||
| * | ||
| * @since 0.1.0 | ||
| * | ||
| * @param string $name The name of the ability. | ||
| * @param array<string,mixed> $properties The properties of the ability. Must include `label`. | ||
| */ | ||
| public function __construct( string $name, array $properties = array() ) { | ||
| parent::__construct( | ||
| $name, | ||
| array( | ||
| 'label' => $properties['label'] ?? '', | ||
| 'description' => $properties['description'] ?? '', | ||
| 'category' => $this->category(), | ||
| 'input_schema' => $this->input_schema(), | ||
| 'output_schema' => $this->output_schema(), | ||
| 'execute_callback' => array( $this, 'execute_callback' ), | ||
| 'permission_callback' => array( $this, 'permission_callback' ), | ||
| 'meta' => $this->meta(), | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the category of the ability. | ||
| * | ||
| * @since 0.1.0 | ||
| * | ||
| * @return string The category of the ability. | ||
| */ | ||
| abstract protected function category(): string; | ||
|
|
||
| /** | ||
| * Returns the input schema of the ability. | ||
| * | ||
| * @since 0.1.0 | ||
| * | ||
| * @return array<string, mixed> The input schema of the ability. | ||
| */ | ||
| abstract protected function input_schema(): array; | ||
|
|
||
| /** | ||
| * Returns the output schema of the ability. | ||
| * | ||
| * @since 0.1.0 | ||
| * | ||
| * @return array<string, mixed> The output schema of the ability. | ||
| */ | ||
| abstract protected function output_schema(): array; | ||
|
|
||
| /** | ||
| * Executes the ability with the given input arguments. | ||
| * | ||
| * @since 0.1.0 | ||
| * | ||
| * @param mixed $input The input arguments to the ability. | ||
| * @return mixed|\WP_Error The result of the ability execution, or a WP_Error on failure. | ||
| */ | ||
| abstract protected function execute_callback( $input ); | ||
|
|
||
| /** | ||
| * Checks whether the current user has permission to execute the ability with the given input arguments. | ||
| * | ||
| * @since 0.1.0 | ||
| * | ||
| * @param mixed $input The input arguments to the ability. | ||
| * @return bool|\WP_Error True if the user has permission, WP_Error otherwise. | ||
| */ | ||
| abstract protected function permission_callback( $input ); | ||
|
|
||
| /** | ||
| * Returns the meta of the ability. | ||
| * | ||
| * @since 0.1.0 | ||
| * | ||
| * @return array<string, mixed> The meta of the ability. | ||
| */ | ||
| abstract protected function meta(): array; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| <?php | ||
| /** | ||
| * Title generation feature implementation. | ||
| * | ||
| * @package WordPress\AI | ||
| */ | ||
|
|
||
| declare( strict_types=1 ); | ||
|
|
||
| namespace WordPress\AI\Features\Title_Generation; | ||
|
|
||
| use WordPress\AI\Abilities\Title_Generation as Title_Generation_Ability; | ||
| use WordPress\AI\Abstracts\Abstract_Feature; | ||
|
|
||
| /** | ||
| * Title generation feature. | ||
| * | ||
| * @since 0.1.0 | ||
| */ | ||
| class Title_Generation extends Abstract_Feature { | ||
|
|
||
| /** | ||
| * {@inheritDoc} | ||
| * | ||
| * @since 0.1.0 | ||
| * | ||
| * @return array{id: string, label: string, description: string} Feature metadata. | ||
| */ | ||
| protected function load_feature_metadata(): array { | ||
| return array( | ||
| 'id' => 'title-generation', | ||
| 'label' => __( 'Title Generation', 'ai' ), | ||
| 'description' => __( 'Generates title suggestions from content', 'ai' ), | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * {@inheritDoc} | ||
| * | ||
| * @since 0.1.0 | ||
| */ | ||
| public function register(): void { | ||
| add_action( 'wp_abilities_api_init', array( $this, 'register_abilities' ) ); | ||
| } | ||
|
|
||
| /** | ||
| * Registers any needed abilities. | ||
| * | ||
| * @since 0.1.0 | ||
| */ | ||
| public function register_abilities(): void { | ||
| wp_register_ability( | ||
| 'ai/' . $this->get_id(), | ||
| array( | ||
| 'label' => $this->get_label(), | ||
| 'description' => $this->get_description(), | ||
| 'ability_class' => Title_Generation_Ability::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.
I think this topic came up in the initial repo scaffolding PR, but I don't understand why we should use abilities for something like this.
My understanding is that abilities are primarily meant for AI models to use WordPress, not that abilities use AI.
As such, I consider title generation a "feature". There will already be an ability to set the post title at some point, and that's the kind of thing I think should be exposed as an ability. That way, an agent could already generate a post title because the generative aspect of it is naturally covered.
Now, I may be mistaken here, and maybe anything you can do in WordPress should be an ability. But thinking from an AI tooling perspective, it feels a bit odd to me to have a tool more or less just to call a generative model when those tools are typically used by generative models anyway.
It would be great to get other leads to chime in here with their perspective on this. @Jameswlepage @swissspidy @JasonTheAdams
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 can easily change this back to where each individual feature registers it's own REST API endpoint that will be used when the feature is triggered (that's initially what I started with).
That said, one of the reasons I ended up switching from that approach here is the Abilities API gives us all of that with basically the same code (registering, permission callbacks, execute callbacks, schema, etc). But in going with the ability approach, we accomplish a few additional things:
Overall my goal here has been to keep things as extendable as possible, provide examples of how these AI tools can be used and hopefully inspire others to build their own things using these same tools (or build on top of what we do). Worth noting this is a slightly different approach then if I was trying to get something merged to WordPress core but leaning in to the "experiments" label here.
All that said, happy to change this approach if we think that's best. Really just want to get this unblocked so we can start to make some more serious progress here.
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.
This all makes sense. I think the main caveat is regarding:
Is the way the Abilities API is used here a good example of how it should be used or not? Going back to my original concern. That's where I'd like other people's feedback so that we can make a decision.
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.
@felixarntz who else can we tag in for review here to get to a resolution and keep feature development unblocked in the plugin?
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.
@jeffpaul Who I pinged above :)
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.
From the WordPress source code[1]:
To me, this makes it clear that abilities are about exposing functionality of WordPress (or plugins) but it leaves open the question of if that functionality can use an external source such as AI or any 3rd party API. Abilities aren't just about exposing functionality to AI, they can also be used (theoretically) in place such as wp-cli, command palate, or in a UI.
I think that generating titles is a discrete activity that can have a defined input, output, permissions, and execution and thus this is an acceptable use of the abilities API.
[1] I see a few specific word choices that I think need to get cleared up with this definition and will be opening a PR for that, but in general I think this is a good definition to use for now.
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.
It's certainly confusing when a tool or ability itself calls another AI tool. Especially when nothing of that is implemented yet and this is just boilerplate code.
This reminds me of a situation in the OG WP-CLI MCP implementation, where we implemented featured image generation as an MCP tool, and that tool made an AI call to generate an image. Ideally that wouldn't have been an MCP tool though, but done by the AI.
Likewise, the AI can already generate titles itself. It doesn't have to call a tool that calls an AI to generate titles. So in this context, a title generation tool call doesn't make sense. A Title Generation UI feature in the editor could generate a title using the in-browser AI model or call the WP PHP AI SDK REST API endpoint to do this work and then use the "Update Post" ability to update the title (or just use the editor APIs).
At the same time, however, I can also see how it's useful for a Title Generation ability to do all of that, so that any plugin could easily integrate the same functionality in their UI without reinventing the wheel.
Soo... both could be true? :-)
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.
The committed definition that @aaronjorbin referenced is explicit and intentional; at the risk of being a broken record the Abilities API is about much more than AI, and definitely more than just MCP.
As far as the delineation between what is an Ability vs a (lowercase) feature vs a(n uppercase) Feature, etc. I think a good part of it is a naming things problem with the word "Features". If we rename to Experiment, we lessen the confusion, and reduce the need for future conversations like this one.
FeaturetoExperiment#70IMO yes, just because we have an Ability doesn't mean that it needs to be exposed or used by MCP, and abilities can also be made up of other abilities. Long term, we could have a Title Generation Ability (with its own schema) that wraps a Content Generation Ability (that itself internally is just a interpolated prompt to an LLM), that are both toggleable via the same Content Generation
FeatureExperiment, with the former usingWP_Ability::$metadata['annotations']['show_in_mcp'] = false, butshow_in_cp = true(or whatever, the implementation details are less important than the concept of composability)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.
It seems that this is a very open-ended conversation that could go on and on.
I agree that Abilities API is "more than AI", yet here we are building it for AI, and I bet it wouldn't have come up as a new API if we weren't building out AI infra for WordPress. But yes, it can and probably should be used for other use-cases than AI agents.
On the other hand, I also agree with Pascal's point that it's weird to have a tool that purely calls an LLM to generate a title because that doesn't make much sense to provide as a tool to an LLM (since it can already do that on its own). For that context, we'd instead need tools to obtain the necessary context (post content etc.).
Maybe the answer is that it's fine for this to be an ability, but it would be an ability that is primarily relevant for other use-cases than AI agents - e.g. simply a way to implement a new WordPress
featureability using a standard API.That brings up an important point though, related to the ongoing conversation how to expose the right tools to an agent: Arguably an ability (tool) like this should not be exposed to an agent (because of the above)? Does there need to be a way to mark such abilities? Or should it simply be handled via ability categories? cc @gziolo for awareness
Anyway, due to the nature of this discussion, I'm happy to unblock this and just stick with the ability implementation for now. The one thing we should revise though is to get rid of the
get_ability_slug()method on theFeatureinterface and base class, to not prematurely couple the two.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.
Hi folks! 👋
I've somehow missed this PR. Thanks, @jeffpaul, for pinging me!
This is a perfectly valid use of Abilities, from my vantage point. I agree with @swissspidy on this one. It would be weird to expose this to MCP and then have an AI call a function that calls AI. In that case, you'd simply have an Ability for updating the post title (or whatever) which is exposed to MCP, and the model calls that while generating the title itself. But to have an ability that uses AI to generate a title is perfectly fine; I'd even be fine if this was two abilities:
The first could potentially even reference the second with a "generate title" boolean input, or something like that. I'm honestly not thinking that hard about it, the point being abilities can be nice and composable. I could even see having a way of piping abilities in the future.
This actually softly brings up something I was thinking over yesterday: introducing an
open-worldannotation to abilities. I'm borrowing this concept from MCP, which is simply a way of saying "by the way, this function reaches outside of the application and does something externally". In this case, it would help clarify that this ability uses an external service — the AI model.In short, I'm good with this!