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

Remove global config for dataclass_wizard #36

Conversation

leifdenby
Copy link
Member

In working on the yaml configuration files in neural-lam I realised that setting global configuration for dataclass_wizard is a very bad idea. This should be obvious because different uses of dataclass_wizard might have different needs for configuration, however previously I didn't realise how to set the config when using dataclass_wizard.YAMLWizard. It turns out the key is to 1) also inherit from dataclass_wizard.JSONWizard (and make this the primary parent class) and 2) use dataclass_wizard.JSONWizard.Meta to define the config (not dataclass_wizard.YAMLWizard.Meta since this class does not exist).

This commit removes the global config for dataclass_wizard and only sets the configuration on mllam-data-preps mllam_data_prep.config.Config config dataclass.

In working on the yaml configuration files in neural-lam I realised that
setting global configuration for `dataclass_wizard` is a very bad idea. This
should be obvious because different uses of `dataclass_wizard` might have
different needs for configuration, however previously I didn't realise how to
set the config when using `dataclass_wizard.YAMLWizard`. It turns out the key
is to 1) also inherit from `dataclass_wizard.JSONWizard` (and make this the
primary parent class) and 2) use `dataclass_wizard.JSONWizard.Meta` to define
the config (not `dataclass_wizard.YAMLWizard.Meta` since this class does not
exist).

This commit removes the global config for `dataclass_wizard` and only sets the
configuration on `mllam-data-preps` `mllam_data_prep.config.Config` config
dataclass.
@leifdenby
Copy link
Member Author

Hopefully the description above is clear @observingClouds, if not let's have a chat tomorrow. I would like for this to get into the next release, so I will bring it to the developer meeting next week.

@observingClouds
Copy link
Contributor

So if I understand correctly, the code here is about ensuring that all unkown parameters in the config files will raise an exception. Recently the GlobalJSONMeta class has been used with the major downside that also other modules loaded at runtime will be affected by this global setting if they use dataclass_wizard.

With this PR you ensure that only the local dataclasses are affected as it now becomes part of the Config dataclass and is no longer global.

Not being familiar with the dataclass_wizard the suggested change is clearer now.

@leifdenby
Copy link
Member Author

So if I understand correctly, the code here is about ensuring that all unkown parameters in the config files will raise an exception. Recently the GlobalJSONMeta class has been used with the major downside that also other modules loaded at runtime will be affected by this global setting if they use dataclass_wizard.

That's exactly right, yes! Thanks again for reviewing this.

@leifdenby leifdenby mentioned this pull request Nov 18, 2024
13 tasks
@leifdenby leifdenby modified the milestones: v0.3.0, v0.5.0 Nov 18, 2024
@joeloskarsson
Copy link
Contributor

Merging this as per roadmap

@joeloskarsson joeloskarsson merged commit 4eceee8 into mllam:main Nov 19, 2024
5 checks passed
@leifdenby
Copy link
Member Author

Merging this as per roadmap

Thanks! Although I hadn't written anything in the changelog yet 😄 But I will do that when I prepare the v0.5.0 release

@joeloskarsson
Copy link
Contributor

Right, sorry about that 😅 Yes, please fix it with the 0.5.0 release. Maybe a good reason to have the PR template also here for MDP?

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

Successfully merging this pull request may close these issues.

3 participants