-
Notifications
You must be signed in to change notification settings - Fork 26
Add the AiClient with testing suite #55
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
Conversation
@felixarntz could you please add the [Feature] label and 'Complete Implementor API' milestone to this PR? This continues the AiClient work from PR #53, which you had previously labeled and milestone-tagged, but I decided to close it and create this separate one as the other one had commits from the other feature |
Add provider availability checking functionality to complete architecture specification. - Add isConfigured(ProviderAvailabilityInterface $availability): bool method - Delegate to ProviderAvailabilityInterface::isConfigured() for consistency
Add base generation method that automatically detects model capabilities and delegates to appropriate generation interfaces. - Add generateResult(prompt, ModelInterface): GenerativeAiResult method - Smart delegation to generateTextResult() or generateImageResult() based on model interface - Comprehensive error handling for unsupported model types - 3 new tests covering text generation, image generation, and error cases
Add streaming text generation using PHP Generator pattern. - Add streamGenerateTextResult() method with Generator return type - Delegate to model's streaming method using yield from - Support model auto-discovery and comprehensive error handling
Add properly typed async operations with model interface validation. - Add generateTextOperation() with TextGenerationModelInterface validation - Add generateImageOperation() with ImageGenerationModelInterface validation - Prefixed operation IDs for better tracking (text_op_, image_op_) - 4 comprehensive tests covering validation and error cases
- Add convertTextToSpeechResult() method with model auto-discovery - Add convertTextToSpeechOperation() method for async processing - Enhance generateResult() to support TextToSpeechConversionModelInterface - Add findSuitableTextToSpeechModel() helper method - Update test expectations for extended generation interface support
- Add generateSpeechResult() method with model auto-discovery - Add generateSpeechOperation() method for async speech processing - Enhance generateResult() to support SpeechGenerationModelInterface - Add findSuitableSpeechModel() helper with speechGeneration capability - Update prompt() method with comprehensive PromptBuilder integration docs
- Add Embedding DTO with vector data representation and dimension tracking - Add EmbeddingResult class extending ResultInterface for embedding results - Add EmbeddingOperation class for async embedding operations - Add EmbeddingGenerationModelInterface for synchronous embedding generation - Add EmbeddingGenerationOperationModelInterface for async embedding operations - Implement generateEmbeddingsResult() method with model auto-discovery - Implement generateEmbeddingsOperation() method for async processing - Add findSuitableEmbeddingModel() helper with embeddingGeneration capability - Support both string[] and Message[] input formats for embeddings - Update PromptBuilder integration documentation to include embeddings
- Create EmbeddingTest.php with 9 test methods covering vector operations and transformations - Create EmbeddingResultTest.php with 12 test methods covering result validation and serialization - Create EmbeddingOperationTest.php with 14 test methods covering all operation states - Create MockEmbeddingGenerationModel.php and MockEmbeddingGenerationOperationModel.php for testing - Add 5 embedding test methods to AiClientTest.php with proper mock integration
- Fix type errors by ensuring proper list types for embedding interfaces - Convert Message arrays to proper lists using array_values() for interface compliance
- Add missing newlines at end of files - Remove trailing whitespace - Fix alphabetical import sorting in test files - All PHPCS violations resolved automatically
- Implement message() method with placeholder that throws RuntimeException - Add comprehensive test coverage for message() method
…ability - Extract PromptNormalizer utility class for prompt format standardization - Extract ModelDiscovery utility class for capability-based model discovery
- Fix list type annotations in PromptNormalizer and AiClient - Ensure all Message arrays are converted to proper lists using array_values() - Remove 4 redundant tests from AiClientTest that are now covered by utility tests - Maintain 100% test coverage while eliminating duplicate test logic - All PHPStan errors resolved, all tests passing
Extract interface validation, operation factory, embedding input normalization, and generation strategy resolution into dedicated utility classes.
…ation - Implement template method pattern in AiClient for generation methods - Add dynamic validation system to InterfaceValidator with configuration mapping - Consolidate EmbeddingInputNormalizer functionality into PromptNormalizer - Refactor operation methods with factory pattern approach - Update all tests to use consolidated normalizer methods
@felixarntz @JasonTheAdams after going back and forth a couple of times over this feature, I can say that I'm satisfied with it right now. Looking forward for your thoughts and improvements |
Adds documentation references to RequirementsUtil and CapabilityUtil (from PR WordPress#64) with usage examples showing integration patterns.
- Fix broken generateResult() method for null model handling - Add model auto-discovery via PromptBuilder delegation - Add generateResultWithCapability() for multi-interface models - Remove redundant utility classes (Models, PromptNormalizer) - Improve error messages with model metadata - Add comprehensive test coverage - Fix coding style issues
- Fix supportsCapability method calls - use getSupportedCapabilities() instead - Update test mocks to use proper ProviderRegistry mocking - Fix capability validation logic with proper capability comparison - Update error message test expectations for improved error formatting - Replace non-existent supportsCapability() with proper capability checking
- Fix mock expectation count in testGenerateResultWithCapabilityUnsupportedCapability - Update exception message format to match capability enum value - Fix code style violations with import sorting and whitespace
Remove over-engineering while preserving full MVP compatibility: - Remove 5 operation methods (not in MVP scope, purely premature) - Remove streaming method (not implemented, throws RuntimeException) - Consolidate duplicate generateResult() logic into single method - Fix broken mock implementations to return actual test data - Remove tests for non-existent methods - Clean up unused imports and code style issues
- Replace complex PHPUnit mock expectations with anonymous classes - Remove global mock setup in favor of test-specific helper methods - Add helper methods following PromptBuilderTest patterns - Remove unused MockTextGenerationModel and MockImageGenerationModel classes - Simplify test code while maintaining same coverage and behavior
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.
@Ref34t Looks pretty good! A few points of follow up feedback where we can simplify, and most importantly enable passing a ModelConfig
.
src/AiClient.php
Outdated
if ($model === null) { | ||
return self::prompt($prompt)->generateResult(); | ||
} | ||
|
||
// Infer capability from model interface (priority order matters) | ||
if ($model instanceof TextGenerationModelInterface) { | ||
return self::generateResultWithCapability($prompt, CapabilityEnum::textGeneration(), $model); | ||
} | ||
|
||
if ($model instanceof ImageGenerationModelInterface) { | ||
return self::generateResultWithCapability($prompt, CapabilityEnum::imageGeneration(), $model); | ||
} | ||
|
||
if ($model instanceof TextToSpeechConversionModelInterface) { | ||
return self::generateResultWithCapability($prompt, CapabilityEnum::textToSpeechConversion(), $model); | ||
} | ||
|
||
if ($model instanceof SpeechGenerationModelInterface) { | ||
return self::generateResultWithCapability($prompt, CapabilityEnum::speechGeneration(), $model); | ||
} | ||
|
||
throw new \InvalidArgumentException( | ||
sprintf( | ||
'Model "%s" must implement at least one supported generation interface ' . | ||
'(TextGeneration, ImageGeneration, TextToSpeechConversion, SpeechGeneration)', | ||
$model->metadata()->getId() | ||
) | ||
); |
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.
We should always rely on PromptBuilder
here, even if a ModelInterface
is provided:
- If the second parameter is a
ModelConfig
, we need to pass it toPromptBuilder::usingModelConfig
. - If a
ModelInterface
is provided, we need to callPromptBuilder::usingModel
before callingPromptBuilder::generateResult
.
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 updated this to work like the other methods where it simply relies on the PromptBuilder
to figure things out. Happily, it did point out a bug in the builder where it didn't infer the capability from the model if provided, which was resolved in 403e3f3.
…er support • Enhanced AiClient class-level documentation with detailed model specification approaches • Added ModelInterface|ModelConfig|null parameter support across all generation methods • Implemented intelligent parameter validation with descriptive error messages • Created shared MockModelCreationTrait for consistent test infrastructure • Added comprehensive ModelConfig parameter testing with various configurations • Implemented data providers for improved test organization and coverage • Added helper method tests using reflection for private method validation • Enhanced test assertions with better specificity and error context • Refactored mock registry setup to reduce code duplication • Updated provider registration comments for better clarity
@felixarntz @JasonTheAdams I believe now that the PR is in good shape. Kindly let me know if you have any other thoughts |
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 helped move this along based on the feedback. I think we should be just about there! Awaiting a (hopefully) final review from @felixarntz!
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.
@Ref34t @JasonTheAdams Thank you for all the iterations. Looks good to go 🎉
This introduces the AiClient, which is intended to be the primary face for Implementors to construct and execute prompts. See the "Traditional API" examples in the code examples in the Architecture doc.
This provides the foundation that the PromptBuilder (PR #49 ) depends on to be complete.