Add the Title Generation feature base #61
Conversation
… an ability. Make a few updates to our base REST endpoint
…for the same purpose
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #61 +/- ##
============================================
+ Coverage 48.48% 58.02% +9.53%
- Complexity 45 67 +22
============================================
Files 6 9 +3
Lines 198 324 +126
============================================
+ Hits 96 188 +92
- Misses 102 136 +34
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:
|
…ty for the title generation ability into the new ability class
…st we will use the content from that. And a number from 1 to 10 to control how many titles we generate
|
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. |
|
Noting that due to waiting on updates in the WP AI client package and the admin settings screen implementation it doesn't yet seem feasible to test this via Playground here. As such, if @dkotter feels this is |
| * | ||
| * @since 0.1.0 | ||
| */ | ||
| class Title_Generation extends Abstract_Ability { |
There was a problem hiding this comment.
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.
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:
- We showcase how the Abilities API can be used, which I believe is one of the goals of this plugin
- We open up avenues for others to more easily build on top of this plugin. For example, if I wanted to build a plugin on top of this one that allows you to generate titles from the post list screen, I would just need to build my own UI then use the Abilities API to trigger the generate titles ability, either by calling the ability via PHP, calling the REST endpoint directly or eventually calling the client-side ability.
- If desired, we can expose this ability to an MCP server (that part isn't done in this PR). While I agree it feels odd to have an ability that an AI tool could call that would then itself call an LLM (why not just skip the middleman in most situations) I could see a scenario where someone is wanting to manage their WordPress content within something like Claude, where it can connect to your site, find content, suggest some titles and then update that content with the title you choose
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.
This all makes sense. I think the main caveat is regarding:
We showcase how the Abilities API can be used, which I believe is one of the goals of this plugin
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.
@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.
From the WordPress source code[1]:
The Abilities API provides a unified, extensible framework for registering and executing discrete capabilities within WordPress. An "ability" is a self-contained unit of functionality with defined inputs, outputs, permissions, and execution logic.
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.
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.
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.
Soo... both could be true? :-)
IMO 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 Feature Experiment, with the former using WP_Ability::$metadata['annotations']['show_in_mcp'] = false, but show_in_cp = true (or whatever, the implementation details are less important than the concept of composability)
There was a problem hiding this comment.
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 feature ability 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 the Feature interface and base class, to not prematurely couple the two.
There was a problem hiding this comment.
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:
- An ability for updating posts
- An ability for generating and returning a title string using AI
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-world annotation 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!
| * | ||
| * @since 0.1.0 | ||
| */ | ||
| abstract class Abstract_Ability extends WP_Ability { |
There was a problem hiding this comment.
I'm strongly against extending WP_Ability like this.
If there's justification for an abstract class, then we should follow the pattern of Abstract_REST_Controller, and use this class to build the args that get passed to a wp_register_ability() call.
There was a problem hiding this comment.
We have discussed this many times before: It's fine to extend WP_Ability, it's allowed by the API. This is a personal preference.
There was a problem hiding this comment.
Except we're in the WordPress/ai repo so it's not only personal preference, and just because something is "allowed" doesn't means it should be done without defined intent, let alone encouraged, or in this case enforced.
(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:
-
Why do we need an abstract wrapper for registering abilities at all, especially while there is (currently) only one ability in this codebase?
-
Why is extending
WP_Abilitythe ideal path to accomplish No. 1?
The two biggest downsides of extending WP_Ability in our particular case (as noted, the general issues with the pattern have been repeatedly discussed):
- Instead of dogfooding the entire Abilities API, we're diverging from it, which beyond the testing/future-compat issues, also make it significantly harder to "graduate" an experimental ability to Core.
- Practically with this approach, every feature that registers an ability needs to duplicate the same registration code.
(IMO the Abstract_REST_Controller pattern addresses both of those downsides, although any extra layers of prescriptive abstraction inside a Feature this early should still cause some pause, considering the whole purpose of this repo is to "experiment" )
There was a problem hiding this comment.
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 WP_Ability, my personal preference is to extend it.
To your questions:
- This PR implements some scaffolding. Based on the other discussion, it looks like we're leaning more or less any feature in WordPress can be an ability, so it makes sense to provide scaffolding for how to implement abilities in the scope of this plugin.
- This project is mostly object-oriented, so using classes in general makes sense. If you don't extend
WP_Ability, we'd probably still want one class per ability to keep things decoupled, especially because the features (or experiments) that make use of the abilities are decoupled.WP_Abilityprovides a possible foundation for this class breakdown, so IMO it makes sense to use it.
Many aspects of coding are subjective. Whether this project uses procedural programming or OOP is subjective, and so is whether it extends WP_Ability or does something else. We can reason about it, but there's no "ideal" and no "right or wrong".
There was a problem hiding this comment.
Weighing in here, I kindly suggest we step back from the "is extending WP_Ability good or bad" and such debate. To @felixarntz's point, it's part of the API and so everything here is a perfectly sensible use of that, and even a great way of showing how it can be done in a good, object-oriented manner. I like it.
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.
There was a problem hiding this comment.
Like I said, I don't feel strongly. But there's also something to be said about "strong opinions, loosely held". At some point it's important to make a call and roll with it, even when not everyone agrees. And then it's important to commit to the decision. For example, I still don't fully agree with an ability that's there purely to call an AI model - but I see my argument has some flaws, and while I think the other arguments also have flaws, it just goes to show there's no ideal or perfect solution and it's simply a tradeoff and decision to make. So I'm not going to complain about that further :)
My perspective here is: Because I don't see a real net-positive in either way, it's most pragmatic to go with how @dkotter implemented it. I personally would have implemented it somewhat similarly. Now, if he or someone else wants to invest the additional time to change it, I won't oppose the change. But FWIW I don't see value in it either.
I think from my perspective everything here is said. Curious to get other people's perspective if there are folks with any +/- arguments. But I also think we should unblock this soon.
There was a problem hiding this comment.
I (probably obviously) like the approach taken here but that said, this isn't a hill I'm going to die on. What I really want is to get things unblocked here so progress can continue and if that means changing the approach, I'm happy to do that.
I will say this thread is pretty long at this point and I'm not sure I fully follow what we consider the "ideal" approach but it seems at a minimum:
- Keep the
Abstract_Abilityclass but don't extendWP_Ability - Keep most of the other methods the same in both the
Abstract_Abilityclass and any class that extends that (justTitle_Generationin this case) - Look at adding a registration method in
Abstract_Abilityto make it easier for Abilities to be registered
Does that seem accurate or are there other parts I'm missing that should be modified?
There was a problem hiding this comment.
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:
- Extending
WP_Abilityas a way of showing how to useability_class - Using
wp_register_ability()in a purely, non-cleverly procedural way
This makes me like the way this works even more. The idea of having an Abstract_Ability that doesn't actually extend WP_Ability feels, to me, even more abstract than this, and doesn't illustrate using ability_class. It's just a procedural way of registering that's wrapped in OOP. I'm also not a fan of bringing the Feature into the Ability level, as I think the Ability is a lower-level concept which should be a dependency of the Feature, not the other way around.
In conclusion, my preference is how this currently works because it illustrates ability_class. I would suggest that one of the next features register the Ability in a procedural way, not tucking it into OOP, as a simple example for folks to follow.
There was a problem hiding this comment.
it seems at a minimum:
- Keep the
Abstract_Abilityclass but don't extendWP_Ability- Keep most of the other methods the same in both the
Abstract_Abilityclass and any class that extends that (justTitle_Generationin this case)- Look at adding a registration method in
Abstract_Abilityto make it easier for Abilities to be registeredDoes that seem accurate or are there other parts I'm missing that should be modified?
Yup, that's all of it 🙇. Basically the WP_Rest_Controller pattern, but with ergonomics that make sense for us.
It's just a procedural way of registering that's wrapped in OOP
@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 ability_class inside Abstract_Ability; instead we're demonstrating wp_register_ability() and how to use it in an OOP manner, and testing the entirety of \WP_Ability (futureproofing). A specific experiment that wants to extends \WP_Ability to use ability_class is less code/maintenance. And the need to extend \WP_Ability (e.g. to overload behavior, not to have OOP ergonomics when creating new abilities) is much less likely than wanting WP_Ability that can opt-into to the entire lifecycle and not worry about future-compat or cross-compat with other experiments.
IOW this doesn't preclude ability_class, it's (imo) easier and more robust to illustrate and experiment with extends \WP_Ability when it's intentional versus when its enforced plugin-wide as an Abstract_* pattern.
(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 😅)
There was a problem hiding this comment.
I personally would have implemented it somewhat similarly.
( @felixarntz In fairness one of the main reasons I introduced ability_class was to keep similar with your prior art and minimal effort in adapting for the WordCamp demo. So really you could say it's because you would have (and did) implemented it similarly that the pattern is supported to begin with 😅)
…ty name when registering the Ability
…e Feature label and description when we register the Ability
| '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' ), |
There was a problem hiding this comment.
absint( null ) will return 0 here if no post_id is provided, so Not provided won't be used in that case.
I think it should be something like this (or some variant):
'post_id' => $args['post_id'] ? absint( $args['post_id'] ) : null, or
or
'post_id' => absint( $args['post_id'] ) ?: esc_html__( 'Not provided', 'ai' ),
There was a problem hiding this comment.
Good catch! Yeah, let's fix this before merging. 👍
There was a problem hiding this comment.
I can update that but note this entire return statement will be changed in the next PR (where we'll either return the generated titles or a WP_Error), this is just here for test purposes
There was a problem hiding this comment.
I was just going to comment that the output schema doesn't match the return. If this is just testing purposes, I think it's fine as is for now.
There was a problem hiding this comment.
Fixed post ID output here e0e4873.
And yeah, the output here will change to match the schema in the next PR. If you want, can see that here (noting there are some changes needed there now after changes made in this PR): https://github.com/WordPress/ai/pull/67/files#diff-8fd34de7d808803d2695dbc4ba5bb8d014520a5063a0a428848f14fb133c6815R141-R149
JasonTheAdams
left a comment
JasonTheAdams
left a comment
There was a problem hiding this comment.
Left a few comments, but I think we can merge this! Great work, @dkotter!
…the read capability instead of edit capability
8 participants
What?
Partially Closes #10
Sets up the base feature class for Title Generation, setting things up for future PRs. Note no UI is added here so the only things to test require direct HTTP requests.
Why?
We want to add a feature for Title Generation and this is the start of that work. Decided instead of having everything be in a single PR I'd break things down into smaller chunks in order to hopefully speed up reviews and testing. This first chunk contains the initial feature class that registers an ability to generate one or more titles from content.
There will need to be followup PRs to handle (at least) the following:
How?
0.4.0-rcto0.4.0Title_Generationclass that extends ourAbstract_Featureclass. This feature is registered as one of our defaults and registers one ability right nowAbstract_Abilityclass that can be used to create new WP AbilitiesTitle_Generationabilities class that extends theAbstract_Abilityclass. This ability allows you to pass in either some content or a post ID (which we then pull content from) as well as the number of titles you want generated (currently limited from 1 to 10). The actual generation will be handled in another PRTesting Instructions
There is no UI yet but can be tested by making direct API requests to the various endpoints:
Make an authenticated
GETrequest to the main abilities endpoint and ensure the Title Generation ability shows thereMake an authenticated
GETrequest to the Title Generation endpoint and ensure information about the ability showsMake an authenticated
POSTrequest to the Title Generation run endpoint and ensure the data that is output matches the data you sent inTest 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.