modelkit icon indicating copy to clipboard operation
modelkit copied to clipboard

Improve ModelDependenciesMapping get Method in m

Open tgenin opened this issue 10 months ago • 0 comments

The Get method of ModelDependenciesMapping exhibits ambiguous behavior. Contrary to what one might expect, it is not a dictionary-like get method. Instead, it acts as a getter/caster that raises an exception when a dependency is not present. This can lead to complicated code, especially when certain configurations do not load a dependency.

For instance, instead of writing:

self.my_dependant_model = self.model_depedencies["my_dependant_model"] if "my_dependant_model" in self.model_depedencies else None

It would be more straightforward to write:

self.my_dependant_model = self.model_depedencies.get("my_dependant_model")

To improve the behavior without breaking the "cast" system, I propose the following change:

def get(
    self, key: str, model_type: Optional[Type[ModelDependency]] = None
) -> ModelDependency:
    m = self.models.get(key)
    if m is None:
        return m
    if model_type and not isinstance(m, model_type):
        raise ValueError(f"Model `{m}` is not an instance of {model_type}")
    return cast(ModelDependency, m)

tgenin avatar Mar 31 '25 12:03 tgenin