Skip to content

Avoid listing models for explicit text model IDs #231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chubes4 wants to merge 2 commits into
base: trunk
Choose a base branch
from
+132 −1
Open

Avoid listing models for explicit text model IDs #231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3e79f62
Avoid listing models for explicit text model IDs
chubes4 May 2, 2026
123286e
Move OpenAI-specific explicit-model logic out of OpenAI-compatible base
chubes4 May 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/Providers/ApiBasedImplementation/AbstractApiBasedModelMetadataDirectory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,18 @@ final public function hasModelMetadata(string $modelId): bool
*/
final public function getModelMetadata(string $modelId): ModelMetadata
{
if ($this->hasCache(self::MODELS_CACHE_KEY)) {
$modelsMetadata = $this->getModelMetadataMap();
if (isset($modelsMetadata[$modelId])) {
return $modelsMetadata[$modelId];
}
}

$explicitModelMetadata = $this->createModelMetadataForExplicitModelId($modelId);
Comment on lines 69 to +78

@felixarntz felixarntz Jun 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This being only in getModelMetadata() but not in listModelMetdata() will lead to inconsistent data - so the approach doesn't work correctly in terms of the full API.

I understand this is meant to be used to bypass the need for a request in case an explicit model ID is passed, but that's not controlled by this class. Any code could call getModelMetadata() here, so it'd be confusing if getModelMetadata() returned a different set of metadata than the same model would appear with as part of listModelMetadata().

I think there's a way to implement this in a way that the code here still bypasses the request while ensuring consistent data. E.g. for listModelMetadata() we could make the request first (necessary there!), and then for every single model ID call this method. If the method returns something, override what the API returned - this way it's the same result as here.

There's performance and data integrity to consider on that path - we'd probably want to build in memoization and potentially even caching of this so that:

  • this isn't called for every model every time that listModelMetadata() is called
  • this doesn't create multiple instances for the same model ID within the same request

It's not super trivial, but I think it's possible.

@chubes4 chubes4 Jun 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review and thoughtful comments!

I think this is mainly a problem on slow connections or local environments where the CPU/RAM is constrained. I have not seen it on my live sites but have seen it repeatedly in a local environment inside Studio/WordPress Playground.

I will revisit this based on your feedback and update the PR to consider the broader implications.

if ($explicitModelMetadata !== null) {
return $explicitModelMetadata;
}
Comment on lines +78 to +81

@felixarntz felixarntz Jun 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a bit of a strange design that this will only be used if no cached value has been found, especially since this injected model metadata does not get cached itself. what if the data from the API differs from what's locally defined in the code?

Either we need to cache the result of this too (which I'd have other concerns over though because of cache invalidation) or we need to always run this before the cache logic. If the implementer wants to add some kind of caching themselves for it, they still could.

@chubes4 chubes4 Jun 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if the data from the API differs from what's locally defined in the code?

I think we should run it before cache logic. I'm trying to keep this as simple as possible.


$modelsMetadata = $this->getModelMetadataMap();
if (!isset($modelsMetadata[$modelId])) {
throw new InvalidArgumentException(
Expand Down Expand Up @@ -114,6 +126,22 @@ protected function getBaseCacheKey(): string
return 'ai_client_' . AiClient::VERSION . '_' . md5(static::class);
}

/**
* Creates metadata for an explicit model ID without listing provider models.
*
* Providers whose APIs accept arbitrary/current model IDs can override this to avoid a live list-models request
* when callers already know the model ID they want to instantiate.
*
* @since n.e.x.t
*
* @param string $modelId The explicit model ID.
* @return ModelMetadata|null The model metadata, or null to fall back to listing provider models.
*/
protected function createModelMetadataForExplicitModelId(string $modelId): ?ModelMetadata
{
return null;
}

/**
* Sends the API request to list models from the provider and returns the map of model ID to model metadata.
*
Expand Down
68 changes: 68 additions & 0 deletions tests/unit/Providers/ApiBasedImplementation/AbstractApiBasedModelMetadataDirectoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@

use InvalidArgumentException;
use PHPUnit\Framework\TestCase;
use WordPress\AiClient\AiClient;
use WordPress\AiClient\Providers\Models\DTO\ModelMetadata;
use WordPress\AiClient\Tests\mocks\MockCache;

/**
* @covers \WordPress\AiClient\Providers\ApiBasedImplementation\AbstractApiBasedModelMetadataDirectory
Expand All @@ -28,6 +30,13 @@ protected function setUp(): void
];
}

protected function tearDown(): void
{
AiClient::setCache(null);

parent::tearDown();
}

/**
* Tests listModelMetadata() method.
*
Expand Down Expand Up @@ -69,6 +78,65 @@ public function testGetModelMetadata(): void
$this->assertSame($this->mockModels['model-1'], $directory->getModelMetadata('model-1'));
}

/**
* Tests getModelMetadata() returns explicit metadata without listing models.
*
* @return void
*/
public function testGetModelMetadataReturnsExplicitMetadataWithoutListingModels(): void
{
$explicitModelMetadata = $this->createStub(ModelMetadata::class);
$explicitModelMetadata->method('getId')->willReturn('explicit-model');
$directory = new MockApiBasedModelMetadataDirectory([], $explicitModelMetadata);

$this->assertSame($explicitModelMetadata, $directory->getModelMetadata('explicit-model'));
$this->assertSame(0, $directory->getListRequestCount());
}

/**
* Tests cached model metadata is preferred over explicit metadata.
*
* @return void
*/
public function testGetModelMetadataPrefersCachedMetadataOverExplicitMetadata(): void
{
$cache = new MockCache();
$cacheKey = 'ai_client_' . AiClient::VERSION . '_' . md5(MockApiBasedModelMetadataDirectory::class) . '_models';
$cachedModelMetadata = $this->createStub(ModelMetadata::class);
$cachedModelMetadata->method('getId')->willReturn('explicit-model');
$cache->seed($cacheKey, ['explicit-model' => $cachedModelMetadata]);
AiClient::setCache($cache);

$explicitModelMetadata = $this->createStub(ModelMetadata::class);
$explicitModelMetadata->method('getId')->willReturn('explicit-model');
$directory = new MockApiBasedModelMetadataDirectory([], $explicitModelMetadata);

$this->assertSame($cachedModelMetadata, $directory->getModelMetadata('explicit-model'));
$this->assertSame(0, $directory->getListRequestCount());
}

/**
* Tests cached metadata misses can still fall back to explicit metadata.
*
* @return void
*/
public function testGetModelMetadataUsesExplicitMetadataAfterCachedMetadataMiss(): void
{
$cache = new MockCache();
$cacheKey = 'ai_client_' . AiClient::VERSION . '_' . md5(MockApiBasedModelMetadataDirectory::class) . '_models';
$otherModelMetadata = $this->createStub(ModelMetadata::class);
$otherModelMetadata->method('getId')->willReturn('other-model');
$cache->seed($cacheKey, ['other-model' => $otherModelMetadata]);
AiClient::setCache($cache);

$explicitModelMetadata = $this->createStub(ModelMetadata::class);
$explicitModelMetadata->method('getId')->willReturn('explicit-model');
$directory = new MockApiBasedModelMetadataDirectory([], $explicitModelMetadata);

$this->assertSame($explicitModelMetadata, $directory->getModelMetadata('explicit-model'));
$this->assertSame(0, $directory->getListRequestCount());
}

/**
* Tests getModelMetadata() method with non-existent model.
*
Expand Down
37 changes: 36 additions & 1 deletion tests/unit/Providers/ApiBasedImplementation/MockApiBasedModelMetadataDirectory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,56 @@ class MockApiBasedModelMetadataDirectory extends AbstractApiBasedModelMetadataDi
*/
private array $mockModels;

/**
* @var ModelMetadata|null
*/
private ?ModelMetadata $explicitModelMetadata;

/**
* @var int
*/
private int $listRequestCount = 0;

/**
* Constructor.
*
* @param array<string, ModelMetadata> $mockModels
*/
public function __construct(array $mockModels = [])
public function __construct(array $mockModels = [], ?ModelMetadata $explicitModelMetadata = null)
{
$this->mockModels = $mockModels;
$this->explicitModelMetadata = $explicitModelMetadata;
}

/**
* @inheritdoc
*/
protected function sendListModelsRequest(): array
{
++$this->listRequestCount;

return $this->mockModels;
}

/**
* @inheritdoc
*/
protected function createModelMetadataForExplicitModelId(string $modelId): ?ModelMetadata
{
if ($this->explicitModelMetadata !== null && $this->explicitModelMetadata->getId() === $modelId) {
return $this->explicitModelMetadata;
}

return parent::createModelMetadataForExplicitModelId($modelId);
}

/**
* Returns the number of list request callbacks.
*
* @return int
*/
public function getListRequestCount(): int
{
return $this->listRequestCount;
}
}
Loading