Adds PromptBuilder preferred model support #110
Conversation
|
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. |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@JasonTheAdams I think there's a bit of a misunderstanding on what a model preference should mean here. See my below feedback.
Probably something that can be quickly addressed though once clarified.
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
I think there's a fundamental flaw in the current logic, it's doing the reverse lookup of what needs to happen for the model preference list to work properly.
src/Builders/PromptBuilder.php
Outdated
| ); | ||
| $candidateModels = []; | ||
| foreach ($modelsMetadata as $modelMetadata) { | ||
| $candidateModels[] = [$this->providerIdOrClassName, $modelMetadata]; |
There was a problem hiding this comment.
Since this can be an ID or a class name, we need to normalize this to always be the ID, in order for the lookup to work.
We already ProviderRegistry::getProviderClassName($id). Maybe we should also have ProviderRegistry::getProviderId($className), and that we could then use here if the value is a class name. That should be fairly straightforward to add.
There was a problem hiding this comment.
I don't think you actually addressed the concern I mentioned above?
|
Ok, @felixarntz I significantly changed how the mapping works. I'm a bit cross-eyed, but I think this is better. Hahah! Here's how it works now:
I'm not really sure how else we'd boost performance, now. I think we're iterating as little as possible. 😄 |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@JasonTheAdams High-level logic looks solid now, there are just a few minor notes. I think the only blocker to merging is the point on properly normalizing the provider class name or ID, so that it's always an ID when using it in a key.
src/Builders/PromptBuilder.php
Outdated
| ); | ||
| $candidateModels = []; | ||
| foreach ($modelsMetadata as $modelMetadata) { | ||
| $candidateModels[] = [$this->providerIdOrClassName, $modelMetadata]; |
There was a problem hiding this comment.
I don't think you actually addressed the concern I mentioned above?
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@JasonTheAdams Awesome, looks great! 🚀
Resolved #103
This adds the
PromptBuilder::usingModelPreference()method which allows the user to provide a list of models it would like to use. If no model is present, then it falls back on the discovery system.