Pydantic config

[benmalef]

Fixes #758

Proposed Changes

• This is the first implementation of parameters configuration using pydantic.
• Please only review it, It is not ready for merging yet.
• It passed the test but may cause problems because it has not defined all the configuration parameters.

Checklist

• CONTRIBUTING guide has been followed.
• PR is based on the current GaNDLF master .
• Non-breaking change (does not break existing functionality): provide as many details as possible for any breaking change.
• Function/class source code documentation added/updated (ensure typing is used to provide type hints, including and not limited to using Optional if a variable has a pre-defined value).
• Code has been blacked for style consistency and linting.
• If applicable, version information has been updated in GANDLF/version.py.
• If adding a git submodule, add to list of exceptions for black styling in pyproject.toml file.
• Usage documentation has been updated, if appropriate.
• Tests added or modified to cover the changes; if coverage is reduced, please give explanation.
• If customized dependency installation is required (i.e., a separate pip install step is needed for PR to be functional), please ensure it is reflected in all the files that control the CI, namely: python-test.yml, and all docker files [1,2,3].
• The logging library is being used and no print statements are left.

[github-actions]

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

[sarthakpati]

Perhaps this check can be simpler, like this?

Suggested change

	if parameters["differential_privacy"] is not None:
	if parameters.get("differential_privacy"):

Would that work?

[sarthakpati]

This file badly needs documentation and comments.

[sarthakpati]

Also, please take a look at the warnings from Codacy to see if any can be fixed: https://app.codacy.com/gh/mlcommons/GaNDLF/pull-requests/976/issues

[benmalef]

@sarthakpati, what do you think about the current approach? Right now, we only use the Parameters object for parsing, and convert to dictionary.

For me, it is better to return a Parameters object with all the necessary parameters, and not a dictionary,
but maybe it requires a lot of changes in the source code.

[sarthakpati]

better to return a Parameters object with all the necessary parameters, and not a dictionary

Can you provide a brief pro/con reasoning as to why this would be the case? Especially since changing to a different data structure would require significant amount of changes throughout the code.

[benmalef]

better to return a Parameters object with all the necessary parameters, and not a dictionary

Can you provide a brief pro/con reasoning as to why this would be the case? Especially since changing to a different data structure would require significant amount of changes throughout the code.

I took a look at the source code, and it is required a lot of changes. So, it is not a good idea. I thought it might offer a better code experience, because Pydantic provides some extra features.

So, as regards the workflow, is it something like this?

[szmazurek]

Hey there!
So, @sarthakpati asked for a few words from my side here. Generally, I think it is a super nice idea to use Pydantic here, as this can automate a lot of checks. I think the approach is ok, and I agree with your todo comments @benmalef in the Parameters class, that some of the fields should themselves be Pydantic models and define their own structure and incorporate ALL possible options that can be specified there (of course, if such list is finite). For example, for the loss function, we can define something like this:

class LossFunction(BaseModel):
    name: Literal["ce","cel" ... ]  # (and so on for all the values available in the losses dict)
    reduction: Optional[Literal["mean", "sum"]] = "mean" # here we allow to specify only those we support in our losses impl and also provide default values
    some_other_param_our_losses_use : Optional[Literal["some_value_we_allow",...] = "some_default_value"
    
class Parameters:
      ....
      loss_function: LossFunction

Not sure if this will work if the loss is only a string without a name keyword, but you get the point.
Having that in such form, the workflow can run as follows:

1. Open yaml and load it as a dict
2. Pass the loaded dict it to the pydantic Parameters object
3. Parameters automatically validates if the values passed are correct and inserts defaults
4. params = pydantic_params_object.model_dump()

And this way we have the parameters in the dict form, validated and ready to use. Additionally, such models hierarchy naturally will become a kind of documentation, where you see all available params and values.

What you guys think? If I was unclear on something ping me, and we can discuss it further. Cheers!

[sarthakpati]

Thanks for the detailed explanation, @szmazurek! I updated your answer with some edits to make it clearer for myself. BTW, since you are working on the Lightning integration, this might be relevant since could perhaps be some overlap between that and this PR. Also, we should do 2 separate tags for them so that we have distinct points of references for the users.

@benmalef: any thoughts on what Szymon has put forth?