Skip to content
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

Kwarg for additional ModelSettings parameters? #272

Open
YanSte opened this issue Dec 16, 2024 · 4 comments
Open

Kwarg for additional ModelSettings parameters? #272

YanSte opened this issue Dec 16, 2024 · 4 comments
Assignees
Labels
enhancement New feature or request

Comments

@YanSte
Copy link
Contributor

YanSte commented Dec 16, 2024

Hi there,

Would it make sense to introduce a kwarg in ModelSettings for this parameter?
We could then include a validation step that checks whether the parameter is included in the model and raise an error.

This would make the model setup more generic and flexible, allowing for easier configuration across different models.

Best.

@sydney-runkle sydney-runkle added the enhancement New feature or request label Dec 16, 2024
@sydney-runkle
Copy link
Member

Hey @YanSte,

Thanks for your question. Indeed, we've been having some discussions about this internally.

One reason we're hesitant to introduce something like extra_settings: dict[str, Any] is that it reduces type safety. With your suggestion above, I'm not quite as concerned bc you mention a validation layer, but this introduces some extra logic to maintain for each model.

Perhaps a happy medium would be to have a model_specific_settings attribute, typed as ModelSpecificSettings, something like:

from typing_extensions import TypedDict

class ModelSpecificSettings(TypedDict):
    anthropic: AnthropicModelSettings
    openai: OpenAIModelSettings
    ...

class AnthropicModelSettings(TypedDict):
    x: int
    y: str
    ....

This way, we retain type checking for those extra settings.

I'll conclude with, I'm not actually opposed to having both model_specific_settings and extra_settings, but we have to acknowledge the type checking tradeoff associated with extra_settings.

@YanSte
Copy link
Contributor Author

YanSte commented Dec 16, 2024

Hi @sydney-runkle,

Thanks for your detailed message and for elaborating.

If I understand at the end you will have ?

ModelSpecificSettings = Union[AnthropicModelSettings, OpenAIModelSettings, ...]

Do you think it will not growing until new compagny will come with new model ? Or new params ?


On my end I thought more about this, giving an extra_param: dict and use a validator compare to class init param.
To validate. So with that we don't extra job for each new provider.

Here an example:

# Example target class for schema inference
# Example usage
try:

    params = ModelSettings(..., extra={"x": 10, "y": "hello", "z": 3.14})
    validator = ParameterValidator(params, target_class=OpenAI) # Mistral ect..
    print("Validated parameters:", validator.params)
except ValueError as e:
    print("Validation error:", e)

This approach ensures robust type checking while minimizing the need for manual updates.

We will have the base settings (temperature etc..) and extra.

What do you think?

@sydney-runkle sydney-runkle self-assigned this Dec 20, 2024
@sydney-runkle
Copy link
Member

Hi @YanSte,

Apologies for the delay. Thanks for your patience over the holidays!

I think it might look more like:

from typing_extensions import TypedDict

class ModelSpecificSettings(TypedDict):
    anthropic: AnthropicSpecificSettings
    openai: OpenAISpecificSettings
    ....

Indeed, I'm in favor of the extra param, though it does come with type checking drawbacks, as none of those extra kwargs can be type checked.

Let's defer to @samuelcolvin re a decision here.

@tostenzel
Copy link

I would also love being able to, e.g., pass a seed to the OpenAI models

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

No branches or pull requests

3 participants