refactor: centralize model requirements logic per Issue #66 #69
Conversation
- Move ModelConfig::toRequiredOptions() → ModelRequirements::fromPromptData() - Move ModelMetadata::meetsRequirements() → ModelRequirements::areMetBy() - Remove PromptBuilder::getModelRequirements() method - Update all call sites to use centralized requirements logic - Add comprehensive test coverage for new methods - Remove unused optimization maps from ModelMetadata Fixes separation of concerns between core API components and model discovery. ModelConfig and ModelMetadata are now pure data holders while ModelRequirements handles all requirement analysis and validation logic.
|
@felixarntz The refractor is ready for review, in your hands |
|
@Ref34t Just spotting this has merge conflicts - would you be able to resolve these please? |
|
@felixarntz will tackle it today sure |
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @[email protected]. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
Conflicts resolved in: - src/Builders/PromptBuilder.php: Removed getModelRequirements() method per Issue WordPress#66 - src/Providers/Models/DTO/ModelConfig.php: Removed toRequiredOptions() method per Issue WordPress#66 - src/Providers/Models/DTO/ModelMetadata.php: Removed meetsRequirements() method per Issue WordPress#66 These methods were moved to ModelRequirements class as part of centralizing model requirements logic.
These test methods were testing the toRequiredOptions() method that was removed as part of Issue WordPress#66 refactoring. The functionality has been moved to ModelRequirements::fromPromptData() method.
- Remove unused OptionEnum import - Fix class closing brace formatting
Update areMetBy() and fromPromptData() method @SInCE annotations from n.e.x.t to 0.2.0 to match the milestone.
|
@felixarntz The merge conflicts have been resolved. kindly review now 🥇 |
There was a problem hiding this comment.
@Ref34t This looks like a solid start!
There are a few areas where maintaining the original logic would be preferable from a performance and architecture perspective. Likely because that's not the case, there are currently a few problems that need to be addressed either way.
| if ($hasFunctionMessageParts) { | ||
| $requiredOptions[] = new RequiredOption(OptionEnum::functionDeclarations(), true); | ||
| } | ||
|
|
||
| // Add input modalities if we have any inputs | ||
| if (!empty($inputModalities)) { | ||
| // Remove duplicates | ||
| $inputModalities = array_unique($inputModalities, SORT_REGULAR); | ||
| $requiredOptions[] = new RequiredOption(OptionEnum::inputModalities(), array_values($inputModalities)); | ||
| } |
There was a problem hiding this comment.
This is missing the logic to ensure the RequiredOption is only added once to the array.
I think this is best solved by reinstating the private includeInRequiredOptions method from before, but now in this class.
| /** | ||
| * Creates ModelRequirements from prompt data and model configuration. | ||
| * | ||
| * @since 0.2.0 |
There was a problem hiding this comment.
While we will very likely add this to 0.2.0, please always use n.e.x.t for any new code. This leaves the version up for a future decision, and we can replace all n.e.x.t instances with the correct version number right before a new release.
| $modelConfig->getOutputSchema() | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
This method is missing several options from other ModelConfig parameters. Please check the original ModelConfig::toRequiredOptions() method and ensure everything is covered here too.
Actually, since this method is now very long, I would suggest you re-implement toRequiredOptions(ModelConfig $modelConfig): array here as a private method, simply to break it up, and since that part is a distinct piece of logic only dependent on the ModelConfig instance provided.
| // Check if all required capabilities are supported | ||
| foreach ($this->requiredCapabilities as $requiredCapability) { | ||
| $supported = false; | ||
| foreach ($metadata->getSupportedCapabilities() as $supportedCapability) { |
There was a problem hiding this comment.
Instead of a nested foreach loop, which can be problematic for performance, why not maintain the original approach of using lookup maps that's present in ModelMetadata::meetsRequirements?
You can call getSupportedCapabilities() and getSupportedOptions() once, in the beginning of this method and then create a $capabilitiesMap and an $optionsMap from them, just like today in ModelMetadata.
This way you can take over the more efficient code as is - which already reduces the risk of introducing new bugs.
Summary
Implements Issue #66 by centralizing model requirements logic into the
ModelRequirementsclass, improving separation of concerns between core API components and model discovery functionality.Changes Made
Move methods to a centralized location:
ModelConfig::toRequiredOptions()→ModelRequirements::fromPromptData()ModelMetadata::meetsRequirements()→ModelRequirements::areMetBy()Remove
PromptBuilder::getModelRequirements()method per issue requirementsUpdate call sites:
PromptBuilder now calls
ModelRequirements::fromPromptData()directlyProviderRegistry now calls
ModelRequirements::areMetBy()Clean up data holder classes:
ModelConfig and ModelMetadata are now pure data holders
Removed unused optimization maps from ModelMetadata
Add comprehensive tests:
4 new tests for
areMetBy()method (matching, missing capability, unsupported values, empty requirements)4 new tests for
fromPromptData()method (text generation, chat history, multimodal input, config options)Architecture Impact
Improved separation of concerns: Core API components no longer depend on model discovery
Single Responsibility:
ModelRequirementshandles all requirements analysisFixes #66