Skip to content

Conversation

@welcoMattic
Copy link
Member

@welcoMattic welcoMattic commented Jul 11, 2025

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
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
Member

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

Copy link
Member

@chr-hertel chr-hertel left a comment

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
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
Copy link
Member

@chr-hertel chr-hertel left a comment

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
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
Copy link
Member

@chr-hertel chr-hertel left a comment

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
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
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