Force a model to be passed when generating embeddings instead of relying on the model resolver - #274
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.
| 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(); | ||
| } |
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')) |
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
left a comment
JasonTheAdams
left a comment
There was a problem hiding this comment.
Thank you, @dkotter! I left a couple of suggestions.
| 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; | ||
| } |
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;
}There was a problem hiding this comment.
Yeah, I missed that exception type. Addressed in ec5fdbc
There was a problem hiding this comment.
I noticed in your commit that you replaced InvalidArgumentException with the new exception. Did you mean to replace it or have it as both?
There was a problem hiding this comment.
@JasonTheAdams Not sure I completely follow? I replaced InvalidArgumentException with AiClientExceptionInterface which I think matches your code example above. Are you suggesting to keep InvalidArgumentException and just add AiClientExceptionInterface as well?
There was a problem hiding this comment.
Sorry, I didn't mean for my example to be an exact drop-in, but I can see how that was confusing. The isSupported() method is meant to always return false. We can either just catch all exceptions through Exception or list out each exceptions, which includes AiClientExceptionInterface and InvalidArgumentException (drilling into nested methods). It may be worth simply doing the former, but I'm not sure if we'd want to prevent unexpected exceptions from getting through. I don't think it's a big deal, so I'm good either way.
There was a problem hiding this comment.
Parsing through our code, it appears that in almost all situations, catching AiClientExceptionInterface is what we'd want here. The one area where this isn't accurate is if a request fails with a status code that isn't in the 3xx-5xx range, that throws a native \RuntimeException and won't currently be caught.
So a couple options here as I see it:
- Widen the catch here to be just
Exception, which catches that one edge case from our code and potential bugs from third party code. But that does mean we hide actual errors that may be better being surfaced - Keep it as
AiClientExceptionInterfaceand look to updateResponseUtil.phpto return our customRuntimeExceptioninstead of the native one, which then ensures all of our code is properly caught - Keep it as
AiClientExceptionInterfaceand not worry about edge cases?
Any preference on these @JasonTheAdams?
There was a problem hiding this comment.
Ah, I was thinking about this wrong. Thanks for the clarification (and you as well, @ColinM-sys). Let's go with 3. The RuntimeException in ResponseUtil is very much a "what on earth?" final catch and should never be reached. If it is we want folks to figure out what happened.
… array{0: string, 1: string}
…e broad support. Add a test to cover this
There was a problem hiding this comment.
Left to small questions, @dkotter, but I believe this is good to go once those are addressed. Please make sure to run the integration tests to confirm it's working.
Thanks for doing this!
|
Re-validated on
On the open
The client's own One nuance that might merit a sentence in the docblock: because provider availability and model metadata are cached for the remainder of the request (as documented), a provider that was reachable earlier in the same request and then becomes unreachable still reports |
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