Skip to content

fix(embedder)!: Use FQCN for embedder model config, to allow any Model child class #99

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

welcoMattic
Copy link
Contributor

Ref: #98

Before:

llm_chain:
    embedder:
        default:
            platform: 'llm_chain.platform.mistral'
            model:
                name: 'Embeddings'
                version: 'mistral-embed'

After

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

This way, any class extending PhpLlm\LlmChain\Platform\Model can be used as embedder model, even one written by users themselves. This way, embeddings model of providers (OpenAI, Mistral, ...) can have the same class name.

@welcoMattic welcoMattic force-pushed the fix/config-embeddings-model-name branch from 4bdf0e5 to 30fbe63 Compare July 3, 2025 14:09
@welcoMattic
Copy link
Contributor Author

I didn't handle the BC here, let me know it this solution is ok. Then, I will add a BC layer to not break user's apps.

@OskarStark
Copy link
Contributor

It is ok to break BC, we don't take BC promise serious until now as we are anyway using 0.x versions

@OskarStark OskarStark changed the title fix(embedder): Use FQCN for embedder model config, to allow any Model child class fix(embedder)!: Use FQCN for embedder model config, to allow any Model child class Jul 4, 2025
@welcoMattic
Copy link
Contributor Author

@chr-hertel
Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants