-
-
Notifications
You must be signed in to change notification settings - Fork 102
[Platform] Introduce ProviderFactory and ProviderConfigFactory #420
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.
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
base: main
Are you sure you want to change the base?
Conversation
bf08cff
to
3055064
Compare
|
||
use Symfony\AI\Platform\Exception\InvalidArgumentException; | ||
|
||
final class Dsn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this code com from? We have several DSN
classes in symfony/symfony
which should be used. I propose to copy it from symonfy/notifer
including tests.
public function getProvider(): string | ||
{ | ||
$scheme = strtolower($this->scheme); | ||
if (str_starts_with($scheme, 'ai+')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for a prefix here
|
||
use Symfony\Contracts\HttpClient\HttpClientInterface; | ||
|
||
final class PlatformFactory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these classes look weird
Glad to have you here @qbejs 👋 😄 |
|
||
## [Unreleased] | ||
|
||
- Introduced `ProviderFactory` and `ProviderConfigFactory` to create AI provider platforms from DSNs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## [Unreleased] | |
- Introduced `ProviderFactory` and `ProviderConfigFactory` to create AI provider platforms from DSNs. | |
- `ProviderFactory` and `ProviderConfigFactory` to create AI provider platforms from DSNs |
return match ($provider) { | ||
'openai' => 'api.openai.com', | ||
'anthropic' => 'api.anthropic.com', | ||
'gemini' => 'generativelanguage.googleapis.com', | ||
'vertex' => 'us-central1-aiplatform.googleapis.com', | ||
'ollama' => 'localhost', | ||
'azure' => throw new InvalidArgumentException('Azure DSN must specify host (e.g. "<resource>.openai.azure.com").'), | ||
default => throw new InvalidArgumentException(\sprintf('Unknown AI provider "%s".', $provider)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those should not be defined in the DSN
class, but rather (like symfony/notfier does) in the AIBundle.php, ofc SfNotfier does this in FrameworkBundle
|
||
if ('azure' === $providerKey) { | ||
$engine = strtolower($config->options['engine'] ?? 'openai'); | ||
$factoryFqcn = match ($engine) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets go with dedicated PlatformFactory classes in each bridge (like the bridges in symfony/symfony)
I'd prefer not to introduce the new term Provider here, but stick to Platform |
This PR implements RFC #402 by introducing a
ProviderFactory
andProviderConfigFactory
to centralize the creation of AI provider platforms (OpenAI, Azure OpenAI, Azure Meta) from DSNs.
What’s new
ProviderConfigFactory::fromDsn()
Parses DSNs into a strongly typed
ProviderConfig
(provider, baseUri, apiKey, options, headers).Supports Azure family selection via
engine
(e.g.openai
,meta
) and common options likedeployment
,version
,timeout
,proxy
,verify_peer
, as well asheaders[...]
.ProviderFactory::fromDsn()
Instantiates the correct bridge
PlatformFactory
(OpenAi, Azure\OpenAi, Azure\Meta) using the parsed config.Sets proper authentication headers per provider (
Authorization: Bearer …
orapi-key
for Azure), and propagatesbase_uri
,options
, andheaders
to the bridge.Unit tests (positive & negative)
Test coverage for both factories, including edge-cases for DSN parsing (e.g. no host → defaults, Azure validation).
A dedicated PHPUnit configuration
phpunit.provider-factory.xml
runs tests that depend on fake bridge factories (see rationale below).A standard suite continues to run with the default
phpunit.xml(.dist)
without interference.Changelog
Entry added in
src/platform/CHANGELOG.md
.Why
This provides a unified, extensible mechanism to construct provider platforms from DSNs.
It makes configuration easier, allows future providers to be added consistently,
and aligns with the design goals outlined in RFC #402.
Why a dedicated PHPUnit config for some tests?
The existing bridges expose static factory methods (e.g.
Bridge\*\PlatformFactory::create(...)
).Static factories are not mock-friendly: traditional PHPUnit doubles cannot intercept static calls and signatures differ per bridge.
To test
ProviderFactory::fromDsn()
deterministically without coupling the suite to real providers, we load fake bridge factories under the same FQCN via a bootstrap that prepends a PSR-4 mapping for theBridge\
namespace.phpunit.provider-factory.xml
) usestests/bootstrap_fake_bridges.php
to ensure fake classes are loaded before the real ones.@group pf
) and excluded from the main suite to avoid accidental mixing.This split lets us:
ProviderFactory
in isolation,Proposed follow-up Improvement (separate PR/RFC)
To remove the need for a separate PHPUnit config and fakes in the future, I propose introducing a thin, optional abstraction layer:
BridgePlatformFactoryInterface
(small contract)Adapters per existing bridge would simply delegate to the current static PlatformFactory::create(...).
BridgeFactoryResolverInterface
+ default implementationA registry that maps (providerKey, engine) → the corresponding adapter (injected via DI).
ProviderFactory::fromDsn() would ask the resolver for the correct adapter, then call ->create(...).