Skip to content

Conversation

hovi
Copy link
Contributor

@hovi hovi commented Jun 8, 2025

I want to make my own factory method where I create providers based on provider name and other parameters (like base_url).
I found out, that logic that does it in the library is creating providers with default constructor without possibility to pass anything to it.

PR suggests 2 changes. Either change is enough to add the required flexibility:

  • Create function that returns provider class without instantiating it
  • Add kwargs parameters to infer_provider that are passed to constructor call

@DouweM
Copy link
Collaborator

DouweM commented Jun 10, 2025

@hovi Thanks! The change looks reasonable, but I don't love that we're not using infer_provider with kwargs ourselves anywhere, and that not all provider classes will support the same kwargs, so it won't be known until runtime if the provider name + kwargs combination was valid.

At minimum, I think we need a new test to make sure we don't accidentally refactor this infer + kwarg behavior away.

I'm also curious how your custom factory method is going to make sure it's only called with valid combinations. You mention base_url in your example, which is only supported by OpenAIProvider, HerokuProvider and MistralProvider, so it'd be easy enough to write your own implementation that only handles the providers you care about.

@hovi
Copy link
Contributor Author

hovi commented Jun 11, 2025

Thanks for reply and taking time read my PR! @DouweM
Thinking about what you wrote, you bring great points. I think it's best to leave out the kwargs part. It adds unnecessary complexity and questionable contract to the library.
To solve this properly, some more complex builder pattern would be required and I don't have such ambition.

The just adding get_provider_class is good enough, that adds flexibility without exposing anything weird (besides the new function coming to existence).

To answer your question. My use-case is running gherkin scenarios using behave and scenario parameters such as model are defined in a scenario step as a table. So far I prototyped my own hard-coded implementation, which works for a few combinations I am using now (mainly openai+ollama, anthropic and gemini), but I know testers will want to play around scenarios with a diverse spectrum of LLMs, so eventually the factory is gonna have to be quite flexible. That's why I got digging to see if there's something in the core library that helps me reduce boilerplate code.
get_provider_class will reduce at least the part provider -> provider class

If you agree with adding this new function, I will modify the PR and remove the kwargs bit.

I will create another PR or issue if I come up with a nice strongly typed builder that could work well in the library.

@DouweM
Copy link
Collaborator

DouweM commented Jun 11, 2025

@hovi What do you think about giving infer_provider a new instantiate: bool = True property that you can turn off to just get the class?

@hovi
Copy link
Contributor Author

hovi commented Jun 11, 2025

@DouweM I personally prefer more smaller functions doing one thing rather than adding extra functionality to existing function.

@DouweM
Copy link
Collaborator

DouweM commented Jun 12, 2025

@hovi I'm OK with infer_provider_class as well, as long as we add a test to make sure it's part of the public API and doesn't accidentally get refactored away.

@hovi hovi force-pushed the infer-provider branch from 582a2a3 to 877151f Compare June 12, 2025 23:28
@hovi
Copy link
Contributor Author

hovi commented Jun 12, 2025

Okay @DouweM renamed the method and added the test.

@DouweM DouweM merged commit e3e435e into pydantic:main Jun 13, 2025
18 checks passed
@DouweM
Copy link
Collaborator

DouweM commented Jun 13, 2025

@hovi Thank you!

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

Successfully merging this pull request may close these issues.

2 participants