-
Notifications
You must be signed in to change notification settings - Fork 40
Introduce event dispatching #144
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
|
|
||
| namespace WordPress\AiClient; | ||
|
|
||
| use Psr\EventDispatcher\EventDispatcherInterface; | ||
| use WordPress\AiClient\Builders\PromptBuilder; | ||
| use WordPress\AiClient\Common\Exception\InvalidArgumentException; | ||
| use WordPress\AiClient\Common\Exception\RuntimeException; | ||
|
|
@@ -90,6 +91,11 @@ class AiClient | |
| */ | ||
| private static ?ProviderRegistry $defaultRegistry = null; | ||
|
|
||
| /** | ||
| * @var EventDispatcherInterface|null The event dispatcher for prompt lifecycle events. | ||
| */ | ||
| private static ?EventDispatcherInterface $eventDispatcher = null; | ||
|
|
||
| /** | ||
| * Gets the default provider registry instance. | ||
| * | ||
|
|
@@ -114,6 +120,56 @@ public static function defaultRegistry(): ProviderRegistry | |
| return self::$defaultRegistry; | ||
| } | ||
|
|
||
| /** | ||
| * Sets the event dispatcher for prompt lifecycle events. | ||
| * | ||
| * The event dispatcher will be used to dispatch BeforePromptSentEvent and | ||
| * AfterPromptSentEvent during prompt generation. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @param EventDispatcherInterface|null $dispatcher The event dispatcher, or null to disable. | ||
| * @return void | ||
| */ | ||
| public static function setEventDispatcher(?EventDispatcherInterface $dispatcher): void | ||
| { | ||
| self::$eventDispatcher = $dispatcher; | ||
| } | ||
|
|
||
| /** | ||
| * Gets the event dispatcher for prompt lifecycle events. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @return EventDispatcherInterface|null The event dispatcher, or null if not set. | ||
| */ | ||
| public static function getEventDispatcher(): ?EventDispatcherInterface | ||
| { | ||
| return self::$eventDispatcher; | ||
| } | ||
|
|
||
| /** | ||
| * Dispatches an event if an event dispatcher is registered. | ||
| * | ||
| * This is a convenience method that handles the null check internally, | ||
| * only dispatching if a dispatcher has been set via setEventDispatcher(). | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @template T of object | ||
| * @param T $event The event to dispatch. | ||
| * @return T The event (potentially modified by listeners). | ||
| */ | ||
| public static function dispatchEvent(object $event): object | ||
| { | ||
| if (self::$eventDispatcher !== null) { | ||
| /** @var T */ | ||
| return self::$eventDispatcher->dispatch($event); | ||
| } | ||
|
|
||
| return $event; | ||
| } | ||
|
Comment on lines
+123
to
+171
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. Instead of having this in That new class could receive an |
||
|
|
||
| /** | ||
| * Checks if a provider is configured and available for use. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,8 +4,11 @@ | |
|
|
||
| namespace WordPress\AiClient\Builders; | ||
|
|
||
| use WordPress\AiClient\AiClient; | ||
| use WordPress\AiClient\Common\Exception\InvalidArgumentException; | ||
| use WordPress\AiClient\Common\Exception\RuntimeException; | ||
| use WordPress\AiClient\Events\AfterPromptSentEvent; | ||
| use WordPress\AiClient\Events\BeforePromptSentEvent; | ||
| use WordPress\AiClient\Files\DTO\File; | ||
| use WordPress\AiClient\Files\Enums\FileTypeEnum; | ||
| use WordPress\AiClient\Messages\DTO\Message; | ||
|
|
@@ -826,7 +829,39 @@ public function generateResult(?CapabilityEnum $capability = null): GenerativeAi | |
|
|
||
| $model = $this->getConfiguredModel($capability); | ||
|
|
||
| // Dispatch BeforePromptSentEvent (allows message modification) | ||
| $beforeEvent = AiClient::dispatchEvent( | ||
|
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. Following up to my above comment: Can we instead access an instance of our new |
||
| new BeforePromptSentEvent($this->messages, $model, $capability) | ||
| ); | ||
| $messages = $beforeEvent->getMessages(); | ||
|
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. Why do we want to allow modifying the messages here? I would prefer if we didn't allow that, at least not from the start. We can always add it later if there's a need for it. I think for now I'd prefer that we kept these events "immutable", I.e. more like an action in WordPress, not a filter. |
||
|
|
||
| // Route to the appropriate generation method based on capability | ||
| $result = $this->executeModelGeneration($model, $capability, $messages); | ||
|
|
||
| // Dispatch AfterPromptSentEvent | ||
| AiClient::dispatchEvent( | ||
| new AfterPromptSentEvent($messages, $model, $capability, $result) | ||
| ); | ||
|
|
||
| return $result; | ||
| } | ||
|
|
||
| /** | ||
| * Executes the model generation based on capability. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @param ModelInterface $model The model to use for generation. | ||
| * @param CapabilityEnum $capability The capability to use. | ||
| * @param list<Message> $messages The messages to send. | ||
| * @return GenerativeAiResult The generated result. | ||
| * @throws RuntimeException If the model doesn't support the required capability. | ||
| */ | ||
| private function executeModelGeneration( | ||
| ModelInterface $model, | ||
| CapabilityEnum $capability, | ||
| array $messages | ||
| ): GenerativeAiResult { | ||
| if ($capability->isTextGeneration()) { | ||
| if (!$model instanceof TextGenerationModelInterface) { | ||
| throw new RuntimeException( | ||
|
|
@@ -836,7 +871,7 @@ public function generateResult(?CapabilityEnum $capability = null): GenerativeAi | |
| ) | ||
| ); | ||
| } | ||
| return $model->generateTextResult($this->messages); | ||
| return $model->generateTextResult($messages); | ||
| } | ||
|
|
||
| if ($capability->isImageGeneration()) { | ||
|
|
@@ -848,7 +883,7 @@ public function generateResult(?CapabilityEnum $capability = null): GenerativeAi | |
| ) | ||
| ); | ||
| } | ||
| return $model->generateImageResult($this->messages); | ||
| return $model->generateImageResult($messages); | ||
| } | ||
|
|
||
| if ($capability->isTextToSpeechConversion()) { | ||
|
|
@@ -860,7 +895,7 @@ public function generateResult(?CapabilityEnum $capability = null): GenerativeAi | |
| ) | ||
| ); | ||
| } | ||
| return $model->convertTextToSpeechResult($this->messages); | ||
| return $model->convertTextToSpeechResult($messages); | ||
| } | ||
|
|
||
| if ($capability->isSpeechGeneration()) { | ||
|
|
@@ -872,15 +907,13 @@ public function generateResult(?CapabilityEnum $capability = null): GenerativeAi | |
| ) | ||
| ); | ||
| } | ||
| return $model->generateSpeechResult($this->messages); | ||
| return $model->generateSpeechResult($messages); | ||
| } | ||
|
|
||
| if ($capability->isVideoGeneration()) { | ||
| // Video generation is not yet implemented | ||
| throw new RuntimeException('Output modality "video" is not yet supported.'); | ||
| } | ||
|
|
||
| // TODO: Add support for other capabilities when interfaces are available | ||
|
Comment on lines
-879
to
-883
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. Why remove these comments? |
||
| throw new RuntimeException( | ||
| sprintf('Capability "%s" is not yet supported for generation.', $capability->value) | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace WordPress\AiClient\Events; | ||
|
|
||
| use WordPress\AiClient\Messages\DTO\Message; | ||
| use WordPress\AiClient\Providers\Models\Contracts\ModelInterface; | ||
| use WordPress\AiClient\Providers\Models\Enums\CapabilityEnum; | ||
| use WordPress\AiClient\Results\DTO\GenerativeAiResult; | ||
|
|
||
| /** | ||
| * Event dispatched after a prompt has been sent to the AI model and a response received. | ||
| * | ||
| * This event allows listeners to inspect the result of the model call for logging, | ||
| * analytics, or other post-processing purposes. The result object is immutable. | ||
| * | ||
| * @since n.e.x.t | ||
| */ | ||
| class AfterPromptSentEvent | ||
|
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. How about instead naming this |
||
| { | ||
| /** | ||
| * @var list<Message> The messages that were sent to the model. | ||
| */ | ||
| private array $messages; | ||
|
|
||
| /** | ||
| * @var ModelInterface The model that processed the prompt. | ||
| */ | ||
| private ModelInterface $model; | ||
|
|
||
| /** | ||
| * @var CapabilityEnum|null The capability that was used for generation. | ||
| */ | ||
| private ?CapabilityEnum $capability; | ||
|
|
||
| /** | ||
| * @var GenerativeAiResult The result from the model. | ||
| */ | ||
| private GenerativeAiResult $result; | ||
|
|
||
| /** | ||
| * Constructor. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @param list<Message> $messages The messages that were sent to the model. | ||
| * @param ModelInterface $model The model that processed the prompt. | ||
| * @param CapabilityEnum|null $capability The capability that was used for generation. | ||
| * @param GenerativeAiResult $result The result from the model. | ||
| */ | ||
| public function __construct( | ||
| array $messages, | ||
| ModelInterface $model, | ||
| ?CapabilityEnum $capability, | ||
| GenerativeAiResult $result | ||
| ) { | ||
| $this->messages = $messages; | ||
| $this->model = $model; | ||
| $this->capability = $capability; | ||
| $this->result = $result; | ||
| } | ||
|
|
||
| /** | ||
| * Gets the messages that were sent to the model. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @return list<Message> The messages. | ||
| */ | ||
| public function getMessages(): array | ||
| { | ||
| return $this->messages; | ||
| } | ||
|
|
||
| /** | ||
| * Gets the model that processed the prompt. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @return ModelInterface The model. | ||
| */ | ||
| public function getModel(): ModelInterface | ||
| { | ||
| return $this->model; | ||
| } | ||
|
|
||
| /** | ||
| * Gets the capability that was used for generation. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @return CapabilityEnum|null The capability, or null if not specified. | ||
| */ | ||
| public function getCapability(): ?CapabilityEnum | ||
| { | ||
| return $this->capability; | ||
| } | ||
|
|
||
| /** | ||
| * Gets the result from the model. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @return GenerativeAiResult The result. | ||
| */ | ||
| public function getResult(): GenerativeAiResult | ||
| { | ||
| return $this->result; | ||
| } | ||
| } | ||
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 wonder whether we want to be a bit safer here.
I don't love that an event listener can arbitrarily replace the entire instance with another instance. I think we'll want to remain in control of what can and what cannot be modified.
While PSR-14 doesn't mandate that, it doesn't mean we cannot be stricter. I think we should include here a check to ensure that the return value is
$modified_event === $event, and otherwise throw.Yes, that technically means we wouldn't even need to return it, since objects are modified by reference. But I think this is preferable since that leaves us in control.
For example, right now you have a
setMessagesmethod on the event implementations, but having that doesn't even matter, since a listener could just create a new instance of the class and replace everything. I think that's not great.