Skip to content

Conversation

sonnymilton
Copy link
Contributor

@sonnymilton sonnymilton commented Sep 30, 2025

Q A
Bug fix? no
New feature? yes
Docs? no
Issues -
License MIT

This PR extends the Ollama\ModelCatalog to support model names that include a size suffix after a colon (e.g. qwen3:32b, all-minilm:33m).

  • The getModel() method now resolves the model by its base name (before the : suffix).
  • The actual name including the suffix is still passed to the model constructor.
  • Additional test cases have been added to ensure models with size suffixes are correctly resolved.

@carsonbot carsonbot added Feature New feature Platform Issues & PRs about the AI Platform component Status: Needs Review labels Sep 30, 2025
@OskarStark OskarStark changed the title [Platform][OLLAMA] Add support for models with size in Ollama model catalog [Platform][Ollama] Add support for models with size in model catalog Oct 1, 2025
@sonnymilton sonnymilton requested a review from OskarStark October 1, 2025 07:19
yield 'nomic-embed-text' => ['nomic-embed-text', Ollama::class, [Capability::INPUT_MESSAGES, Capability::OUTPUT_TEXT, Capability::OUTPUT_STRUCTURED, Capability::INPUT_MULTIPLE]];
yield 'bge-m3' => ['bge-m3', Ollama::class, [Capability::INPUT_MESSAGES, Capability::OUTPUT_TEXT, Capability::OUTPUT_STRUCTURED, Capability::INPUT_MULTIPLE]];
yield 'all-minilm' => ['all-minilm', Ollama::class, [Capability::INPUT_MESSAGES, Capability::OUTPUT_TEXT, Capability::OUTPUT_STRUCTURED, Capability::INPUT_MULTIPLE]];
yield 'all-minilm:33m' => ['all-minilm:33m', Ollama::class, [Capability::INPUT_MESSAGES, Capability::OUTPUT_TEXT, Capability::OUTPUT_STRUCTURED, Capability::INPUT_MULTIPLE]];
Copy link
Contributor

Choose a reason for hiding this comment

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

same for this model


return [
'name' => $actualModelName,
'baseName' => explode(':', $actualModelName, 2)[0],
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need the basename?

Copy link
Contributor Author

@sonnymilton sonnymilton Oct 1, 2025

Choose a reason for hiding this comment

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

We use the basename fallback to avoid duplicating identical model registrations for every size suffix (e.g. qwen3:32b, qwen3:64m).
Almost every model supported by OLLAMA contains variants with size suffix. For example, the qwen3 model supports over 50 different variants. https://ollama.com/library/qwen3/tags
Registering each one of them would lead to a huge increase of the catalog

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but what are you doing with the baseName in user land?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don’t expose or use the baseName in user land.
It’s only an internal fallback inside AbstractModelCatalog::getModel().
From the perspective of user land, they always pass the full name (e.g. qwen3:32b) and get back a model with that exact name. The baseName is just used internally to resolve the model configuration when the variant isn’t explicitly registered.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'baseName' => explode(':', $actualModelName, 2)[0],

so we don't need it here, right? we will end up with model name qwen3:32b which is fine

Copy link
Contributor Author

@sonnymilton sonnymilton Oct 1, 2025

Choose a reason for hiding this comment

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

Since :size suffixes are not a cross-vendor pattern (only use Ollama), it might be more appropriate to keep the fallback logic in the Symfony\AI\Platform\Bridge\Ollama\ModelCatalog::getModel() (like in the first version of this PR 853d902).

@OskarStark
Copy link
Contributor

Ah now I get it, it is working, but you need to create the model to be able to use it, which is fine IMHO. the "sizes" are not common enough through all platforms IMHO. So you should define the model on your end and use it

@sonnymilton
Copy link
Contributor Author

From the user perspective you still need to explicitly register the model you want to use. The fallback only helps when a variant isn’t registered but the base model is.

@sonnymilton
Copy link
Contributor Author

sonnymilton commented Oct 1, 2025

Also worth mentioning: before the recent refactoring (when PlatformCatalog was introduced), this already worked as expected in the Ollama bridge. At that time, the matching of a suitable config was done via regular expressions, which allowed different variants to resolve correctly to the base model. With the refactor this logic was lost, so this PR just reintroduces the same behavior.

@OskarStark
Copy link
Contributor

Fixed in #712 with a slightly different approach. Thanks for proposing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature Platform Issues & PRs about the AI Platform component Status: Needs Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants