feat(model): validate model name configuration

[Pouyanpi]

Description

Implement proper model name validation in the Model class to ensure that a model name is specified exactly once, either directly via the 'model' field or through parameters. This also normalizes the configuration by moving model names from parameters to the dedicated model field.

• Add tests to verify all validation scenarios

• Add log.info to warn the user about this change per Mike's suggestion OR do not pop it from Model

[mikemckiernan]

Unsolicited suggestion: WDYT about warning with a deprecation message if the model is specified in the parameters field? If we could gradually direct folks to use the dedicated model field, that might be simpler in the long run. Sometimes, the straightjacket is the right fit. My $0.02.

[Pouyanpi]

Unsolicited suggestion: WDYT about warning with a deprecation message if the model is specified in the parameters field? If we could gradually direct folks to use the dedicated model field, that might be simpler in the long run. Sometimes, the straightjacket is the right fit. My $0.02.

It is still ok to set model or model_name in parameters field. We get validation error if both model field and any of model or model_name in parameters are set.

But I agree, If someone is using something like model.parameters["model_name"] somewhere in their code this change is problematic because we are popping it from parameters and set it to model, in this case I think adding a warning is useful. Thank you for pointing it out 👍🏻

[tgasser-nv]

The code looks good, and the tests cover all the permutations of config in main model / parameters with model and model_name. I have a couple of questions though:

1. Why do we need to support models in both the yaml config and parameters?
2. Why not standardize on just model rather than both model and model_name?

[Pouyanpi]

The code looks good, and the tests cover all the permutations of config in main model / parameters with model and model_name. I have a couple of questions though:

1. Why do we need to support models in both the yaml config and parameters?

in the parameters user can add any field that the underlying langchain llm accepts. Sadly, not alll the langchain third party integrations (e.g., community models) support a unified signature, some accepts model_name and not model

2. Why not standardize on just model rather than both model and model_name?

it depends on the langchain model and provider used, and in llmrails.py we are checking this explicitly:

        if model_config.model:
            # Some LLM providers use `model_name` instead of `model`.
            # Use the `__fields__` attribute which is computed dynamically by pydantic.
            if "model" in provider_cls.__fields__:
                kwargs["model"] = model_config.model
            if "model_name" in provider_cls.__fields__:
                kwargs["model_name"] = model_config.model

[trebedea]

This is useful, kudos for the tests.

@mikemckiernan , maybe we should make more explicit that we accept both model and model_name in parameters, depending on how langchain providers are naming this key / attribute?

[mikemckiernan]

This is useful, kudos for the tests.

@mikemckiernan , maybe we should make more explicit that we accept both model and model_name in parameters, depending on how langchain providers are naming this key / attribute?

Isn't it that what we really want to emphasize is that the top-level model: field is never wrong? Won't spending words on the flexibility just pin us down longer on supporting too flexible an API?

[Pouyanpi]

The model field's value is checked against the Langchain class:

        if model_config.model:
            # Some LLM providers use `model_name` instead of `model`.
            # Use the `__fields__` attribute which is computed dynamically by pydantic.
            if "model" in provider_cls.__fields__:
                kwargs["model"] = model_config.model
            if "model_name" in provider_cls.__fields__:
                kwargs["model_name"] = model_config.model                

so at the end model "is never wrong" and is the only required field. "model_name" makes it more explicit and also current implementation makes it backward compatible.

Please let me know if there are further objections that must be addressed in this PR. I'd like to merge it tomorrow as it is blocking two other PRs. We can follow up on it in separate PRs.

cc @tgasser-nv @mikemckiernan

[mikemckiernan]

my feedback is never blocking. I appreciate the heads up and the chance to peek at the change. TY!

[trebedea]

My comment here was that here the parameters are actually meant for the Langchain classes that we are using to actually interact with various LLM providers. This way we are able to set any of the attributes different LLM providers support, model and model_name are just two of them, and we cannot control the names of these attributes:

https://github.com/NVIDIA/NeMo-Guardrails/blob/27169c73388b473cd02c30d9dc03dd5a695e24ca/nemoguardrails/rails/llm/llmrails.py#L357

We can merge this, but maybe we should highlight in the docs in another MR that the parameters key in our config.yml file are actually the attributes that can be passed to different Langchain LLM (or anything that extends BaseLanguageModel) providers. Right now in the config documentation we just mention that parameters: any additional parameters, e.g., temperature, top_k, etc. (but there is no mention how to understand which are the actual supported additional parameters, or maybe there is this information somewhere else and I cannot find it now) - @mikemckiernan any thoughts?

https://docs.nvidia.com/nemo/guardrails/latest/user-guides/configuration-guide.html