Force a model to be passed when generating embeddings instead of relying on the model resolver - #274
Open
dkotter wants to merge 8 commits into
Open
Force a model to be passed when generating embeddings instead of relying on the model resolver#274dkotter wants to merge 8 commits into
dkotter wants to merge 8 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.44% -0.06%
- Complexity 1327 1373 +46
============================================
Files 68 69 +1
Lines 4295 4419 +124
============================================
+ Hits 3715 3820 +105
- Misses 580 599 +19
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')) |
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