Force a model to be passed when generating embeddings instead of relying on the model resolver - #274
Open
dkotter wants to merge 10 commits into
Open
Force a model to be passed when generating embeddings instead of relying on the model resolver#274dkotter wants to merge 10 commits into
dkotter wants to merge 10 commits into
Conversation
…ments are unmet so we can provide a more specific error message to a user
…ModelResolutionTrait to better support changes we need in the EmbeddingBuilder
…that will ensure the model provided is valid and will work for the request. Update our helpers in the AiClient to require a model be passed
…der isn't configured
|
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 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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #274 +/- ##
============================================
+ Coverage 86.49% 86.54% +0.05%
- Complexity 1327 1381 +54
============================================
Files 68 69 +1
Lines 4295 4438 +143
============================================
+ Hits 3715 3841 +126
- Misses 580 597 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…pecified, not if an invalid model was provided. This matches what the README claims, where someone can run isSupported to see if a model is supported without worrying about catching exceptions
…rate this out from prepareModel so we only touch model config prior to making a request, not verifying things
There was a problem hiding this comment.
Pull request overview
This PR updates the embedding-generation surface so callers must explicitly specify which model is used (instead of relying on resolver-based auto-selection), and enhances model requirement validation to produce more actionable errors. This aligns with embeddings’ constraint that vectors are only comparable within the same model.
Changes:
- Require an explicit embedding model (via
usingModel()orusingProviderModel()) and validate provider configuration + model capabilities/options before requesting embeddings. - Add
ModelRequirements::getUnmetRequirements()to report all unsupported capabilities/options, enabling more detailed error messages. - Update unit/integration tests, CLI behavior, and documentation to reflect the explicit-model requirement and new failure modes.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/Providers/Models/DTO/ModelRequirementsTest.php | Adds coverage for getUnmetRequirements() and additional areMetBy() edge cases. |
| tests/unit/Builders/EmbeddingBuilderTest.php | Updates embedding builder tests for explicit model requirement, provider-config checks, and option validation behavior. |
| tests/unit/AiClientTest.php | Updates traditional embedding API tests for the new required-model signature and tuple support. |
| tests/traits/MockModelCreationTrait.php | Enhances embedding model metadata helper to declare realistic supported options by default. |
| tests/integration/OpenAi/EmbeddingGenerationIntegrationTest.php | Updates OpenAI integration tests to always specify an embedding model and adds new negative tests. |
| tests/integration/Google/EmbeddingGenerationIntegrationTest.php | Adds Google embedding integration tests with explicit model selection and negative cases. |
| src/Providers/Models/DTO/ModelRequirements.php | Implements getUnmetRequirements() and refactors areMetBy() to use it. |
| src/Builders/Traits/ModelResolutionTrait.php | Refactors trait to focus on model selection and delegate config handling to ModelConfigurationTrait. |
| src/Builders/Traits/ModelConfigurationTrait.php | Introduces shared model-config accumulation/merging for builders. |
| src/Builders/EmbeddingBuilder.php | Reworks builder to require and verify an explicitly named model (no resolver-based discovery). |
| src/AiClient.php | Updates traditional embedding APIs to require a model and adds helper to configure an embedding builder. |
| README.md | Updates embedding docs to require explicit model and explains why; adds discovery example. |
| docs/ARCHITECTURE.md | Updates builder architecture explanation and embedding examples to reflect explicit model verification approach. |
| cli.php | Requires --providerId + --modelId for embedding outputs and updates builder setup accordingly. |
Suppressed comments (2)
src/AiClient.php:470
- For consistency with generateEmbeddingResult() accepting a legacy 3rd-arg registry, generateEmbedding() should also accept the 3-argument form (input, model, registry) without a TypeError.
public static function generateEmbedding(
$input,
$model,
?ModelConfig $modelConfig = null,
?ProviderRegistry $registry = null
): Embedding {
return self::generateEmbeddingResult($input, $model, $modelConfig, $registry)->getEmbedding();
src/AiClient.php:496
- generateEmbeddings() has the same avoidable TypeError risk for existing calls that pass a ProviderRegistry as the 3rd argument. If generateEmbeddingResult() supports the legacy argument ordering, this method should too.
public static function generateEmbeddings(
array $inputs,
$model,
?ModelConfig $modelConfig = null,
?ProviderRegistry $registry = null
): array {
return self::getConfiguredEmbeddingBuilder($inputs, $model, $modelConfig, $registry)
->generateEmbeddings();
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
438
to
446
| public static function generateEmbeddingResult( | ||
| $input, | ||
| $modelOrConfig = null, | ||
| $model, | ||
| ?ModelConfig $modelConfig = null, | ||
| ?ProviderRegistry $registry = null | ||
| ): EmbeddingResult { | ||
| self::validateModelOrConfigParameter($modelOrConfig); | ||
| return self::applyModelOrConfig(self::input($input, $registry), $modelOrConfig) | ||
| return self::getConfiguredEmbeddingBuilder($input, $model, $modelConfig, $registry) | ||
| ->generateEmbeddingResult(); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Yes, this is intentionally a breaking change
| 'PHP powers a large part of the web.', | ||
| 'WordPress makes publishing accessible.', | ||
| ]) | ||
| ->usingModel(GoogleProvider::model('gemini-embedding-001')) |
7 tasks
Tested against a local Ollama (no API keys needed)Since Fueled/ai-provider-for-ollama#69 shipped in 1.2.0, Ollama is a zero-cost way to exercise this PR end-to-end, so I ran it that way instead of the Google/OpenAI path in the testing instructions. Setup
Static / unit
Live integration — 36/36 pass on PHP 8.5 and 7.4
Trunk vs. this branch, same vendor + same Ollama: with no model given,
Full harness output (PHP 8.5.9)Notes (none blocking)
Net: behaves exactly as described, no regressions found across PHP 7.4–8.5, and the only action item is the downstream wrapper change in the AI plugin. |
JasonTheAdams
requested changes
Aug 24, 2026
JasonTheAdams
left a comment
JasonTheAdams
left a comment
Member
There was a problem hiding this comment.
Thank you, @dkotter! I left a couple of suggestions.
| * @param ModelInterface|ModelConfig|null $modelOrConfig Optional specific model to use, | ||
| * or model configuration for auto-discovery, | ||
| * or null for defaults. | ||
| * @param ModelInterface|array{0: string, 1: string} $model The model to use, either as an |
Member
There was a problem hiding this comment.
Instead of doing array{0: string, 1: string} in various places, can we make a type for this and use/import it? That should help clarify what this is without having to explain it in every comment.
Comment on lines
+277
to
+283
| try { | ||
| $model = $this->locateModel(); | ||
| } catch (InvalidArgumentException $e) { | ||
| // The model is unusable: its provider is not registered or configured, or the provider | ||
| // has no model with the given ID. Either way it cannot fulfill the request. | ||
| return false; | ||
| } |
Member
There was a problem hiding this comment.
isSupported()'s docblock promises that "any reason the specified model cannot fulfill the request is reported as false," but this catch only covers InvalidArgumentException. On the usingProviderModel() path, locateModel() → getProviderModel() → getModelMetadata() can trigger the provider's list-models HTTP request (when it isn't already cached by the availability check), and a transport failure there surfaces as a ResponseException — which extends RuntimeException, not InvalidArgumentException, so it escapes isSupported() despite the documented contract.
Since ResponseException and both custom exceptions implement AiClientExceptionInterface, catching that interface here would make the behavior match the docblock:
try {
$model = $this->locateModel();
} catch (AiClientExceptionInterface $e) {
// The model is unusable: its provider is not registered or configured, the provider has no
// model with the given ID, or the provider could not be reached. Either way it cannot
// fulfill the request.
return false;
}
Contributor
Author
There was a problem hiding this comment.
Yeah, I missed that exception type. Addressed in ec5fdbc
… array{0: string, 1: string}
…e broad support. Add a test to cover this
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Instead of relying on the
ModelResolverto determine which model should be used when running theEmbeddingBuilder(which is what thePromptBuilderdoes), require a specific model to be passed in and if not, return an error.Why?
Embedding vectors created by one model aren't compatible with vectors created by another model. For this reason it's important that a specific model is always provided instead of relying on a
ModelResolverto determine this, which has a high likelihood of choosing a different model at various points.The original version of embeddings (added in #244) closely followed what we already do in the
PromptBuilder, but we realized that we should deviate slightly to force a model be passed. This PR makes that update and now an error will be returned if someone tries to use theEmbeddingBuilderwithout passing in a specific model.How?
generatemethods to require a model be passed inModelResolutionTraitinto a newModelConfigurationTraitand use that in ourEmbeddingBuilderEmbeddingBuildersets the model properly and validates that the model is available and will work for the requestgetUnmetRequirementsmethod that will tell us all of the requirements that aren't met, allowing us to provide more detailed error messagesUse of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Opus 5
Used for: Evaluating the existing code, iterating on a plan to make the above changes and then executing that plan. All code was reviewed and tested by me
Testing Instructions
Hard to test this PR on it's own as it requires an AI Provider that supports embeddings. Currently we have upstream PRs that add this support in but those haven't been released yet. Easiest way to test is the following:
.envfile and on one line, addGOOGLE_API_KEY=YOUR KEY HEREand on the next line addOPENAI_API_KEY=YOUR KEY HEREcomposer test:integration(Note there are a couple errors with function calling tests but those are existing issues, not related to this PR)After the above, you can also test directly using our
cli.phpfile:OPENAI_API_KEY=123456 php cli.php 'Your text here' --providerId=openai --modelId=text-embedding-3-small --outputFormat=embedding-jsonYou should see output on the command line that shows the embedding result.
Changelog Entry