Skip to content

[AiBundle] Use model FQCN for indexer config, to allow any Symfony\AI\Platform\Model child class#88

Merged
chr-hertel merged 1 commit into
symfony:mainfrom
welcoMattic:fix/config-embeddings-model-name
Jul 15, 2025
Merged

[AiBundle] Use model FQCN for indexer config, to allow any Symfony\AI\Platform\Model child class#88
chr-hertel merged 1 commit into
symfony:mainfrom
welcoMattic:fix/config-embeddings-model-name

Conversation

@welcoMattic

@welcoMattic welcoMattic commented Jul 11, 2025

Copy link
Copy Markdown
Member
Q A
Bug fix? yes
New feature? no
Docs? no
Issues Fix #59
License MIT

Before:

ai:
    indexer:
        default:
            platform: 'symfony_ai.platform.mistral'
            model:
                name: 'Embeddings'
                version: 'mistral-embed'

After

ai:
    indexer:
        default:
            platform: 'symfony_ai.platform.mistral'
            model:
                class: 'Symfony\AI\Platform\Bridge\Mistral\Embeddings'
                name: !php/const Symfony\AI\Platform\Bridge\Mistral\Embeddings::MISTRAL_EMBED

This way, any class extending Symfony\AI\Platform\Model can be used as embedder model, even one written by users themselves.
Embeddings model of providers (OpenAI, Mistral, Gemini, ...) can be named Embeddings with no issue.


Replace php-llm/llm-chain-bundle#99

@welcoMattic welcoMattic force-pushed the fix/config-embeddings-model-name branch from c6d0eef to 77b48a2 Compare July 11, 2025 13:05
@welcoMattic

Copy link
Copy Markdown
Member Author

Repost original comment from @chr-hertel (php-llm/llm-chain-bundle#99 (comment)):

Yup, that first implementation was def too stupid, true. And I guess the className thing is something that should be supported - especially when you thin about user land model classes for example.

However, looking at this config:

llm_chain:
    embedder:
        default:
            platform: 'llm_chain.platform.mistral'
            model:
                name: 'PhpLlm\LlmChain\Platform\Bridge\Mistral\Embeddings'
                version: 'mistral-embed'

This is a bit redundant, right? the service llm_chain.platform.mistral would anyways only support one embeddings class - I think we could also solve that by the Platform exposing which models (and their names) it would support. so that we would basically only chose from a subset of those classes.

WDYT?

@welcoMattic welcoMattic added Bug Something isn't working AI Bundle Issues & PRs about the AI integration bundle labels Jul 11, 2025
@chr-hertel chr-hertel added the BC Break Breaking the Backwards Compatibility Promise label Jul 12, 2025
@chr-hertel

Copy link
Copy Markdown
Member

Yes, let's do this to unblock the usage for now - we can work on a slimmer config as a next step.

@chr-hertel chr-hertel left a comment

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.

Let's break it once for both parameters and also the agent config - same issue.

What do you think about?

  • class instead of name or className
  • name instead of version (also rather deprecated)

and I guess the docs and demo need an update as well here

@chr-hertel

Copy link
Copy Markdown
Member

Conflicts after merging #94, but rebase should do the trick hopefully

@welcoMattic welcoMattic force-pushed the fix/config-embeddings-model-name branch from 77b48a2 to ed53f59 Compare July 15, 2025 08:30
@welcoMattic welcoMattic requested a review from OskarStark as a code owner July 15, 2025 08:30
@welcoMattic welcoMattic force-pushed the fix/config-embeddings-model-name branch 2 times, most recently from 4fd409b to 10585c9 Compare July 15, 2025 09:07
@welcoMattic welcoMattic requested a review from chr-hertel July 15, 2025 09:09

@chr-hertel chr-hertel left a comment

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.

Two things open from my side:

  1. adopting the config of the model in agent section, so it is consistent:

->scalarNode('name')->isRequired()->end()
->scalarNode('version')->defaultNull()->end()

  1. adopting the change in the demo's configuration, e.g.:

name: 'Embeddings'
version: 'text-embedding-ada-002'

@welcoMattic welcoMattic force-pushed the fix/config-embeddings-model-name branch 2 times, most recently from 6b77e1d to 2ca048f Compare July 15, 2025 12:35
@welcoMattic welcoMattic force-pushed the fix/config-embeddings-model-name branch from 2ca048f to 66b6cbc Compare July 15, 2025 12:38
@welcoMattic

Copy link
Copy Markdown
Member Author

@chr-hertel comment addressed, I made the same changes on Agent configuration.
Docs and demo are also fixed.

@OskarStark OskarStark requested a review from chr-hertel July 15, 2025 13:42

@chr-hertel chr-hertel left a comment

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.

Looks good to me now - nice one! :)

@chr-hertel

Copy link
Copy Markdown
Member

Follow up ideas:

  • Generic model classes should be possible
  • Is the model a service? feels a bit odd since it looks like a DTO but also carries configuration
  • A Platform usually defines already a set of models - why bother to configure it explicitly!?

@chr-hertel

Copy link
Copy Markdown
Member

Thank you @welcoMattic.

@chr-hertel chr-hertel merged commit d8519d2 into symfony:main Jul 15, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI Bundle Issues & PRs about the AI integration bundle BC Break Breaking the Backwards Compatibility Promise Bug Something isn't working Status: Needs Review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[AiBundle] OpenAI Embeddings model used no matter the platform defined on the indexed

3 participants