Add a function to load configurations with correct type hints

[llucax]

When loading configurations from dictionaries, using marshmallow_dataclass, the type hints are not preserved by the load() method, leading to repetitive casting every time load() needs to be used.

This commit adds a new load_config() function to the frequenz.sdk.config module that takes care of loading configurations from dictionaries into configuration classes with correct type hints.

[ela-kotulska-frequenz]

Thanks a lot!

[ela-kotulska-frequenz]

Maybe load_marshmallow_config ?

[llucax]

I'm not sure if it is worth adding all of that to the name of the function, after all the idea was to make the loading short and neat, and it is documented what's expected. It will be a runtime error passing anything that is not a marshmallow_dataclass (actually no, I think a plain dataclass will work too! ¹).

Also, this won't be able to load using a plain marshmallow.Schema, it actually needs a marshmallow_dataclass.dataclass, so if we want to make it unambigous it would have to be at least load_marshmallow_dataclass_config() 😬. At least for me if I read load_marshmallow_config() I would expect it to take a plain marshmallow.Schema.

Footnotes

1. marshmallow_dataclass.class_schema also accept a plain dataclass and gets a schema out of it on the fly. ↩

[llucax]

Actually, I will update the docs and tests to reflect this.

[llucax]

Updated!

[daniel-zullo-frequenz]

I understand the function is internal but IMO the documentation should include the Raises section as it is the only way the caller of the function can handle validation errors.

[llucax]

I agree it could be a good addition, but the docs say very clearly that this is just a wrapper for marshmallow.Schema.load(), so I guess users can infer it from there. I can add it in a separate PR if you think it is worth it though.

[daniel-zullo-frequenz]

I can add it in a separate PR if you think it is worth it though.

No, it's probably not worth it. Though I'd explicitly add it if the function is exposed to the user at some point

[llucax]

It is already exposed via frequenz.sdk.config though :)

[daniel-zullo-frequenz]

oops, would you mind to add it in a different PR then? 👼

I have some other concern that I'd like to mention before adding any Raise docs.

def test_none_settings() -> None:
    """Test loading None settings."""
    config: dict[str, Any] = {"key": "value"}

    text: str = "text"

    with pytest.raises(marshmallow.ValidationError) as exc_info:
        loaded_config = load_config(SimpleConfig, None)  # mypy complains incompatible type "None";

    with pytest.raises(marshmallow.ValidationError) as exc_info:
        loaded_config = load_config(
            SimpleConfig, config.get("unknown", None)
        )  # mypy is happpy with `None` when `get()` is used

Since mypy doesn’t raise any issues when get() is used, we can assume marshmallow will handle it. Marshmallow will raise ValidationError for the None case if default marshmallow parameters are used but that might be different when non-default parameters are set, for instance, unknown=INCLUDE will make marshmallow to raise a TypeError if None is passed as a config.

[llucax]

Just for the record, we had a quick call and decided it would be impossible to document every pitfall we can have with marshmallow in this function, so we'll leave it like this. If we touch this code again, the docs can be made a bit more clear about this being just a wrapper over marshmallow.Schema.load().