Skip to content
This repository has been archived by the owner on Nov 21, 2022. It is now read-only.

Add DataClass Hydra Support #40

Closed
SeanNaren opened this issue Jan 13, 2021 · 5 comments
Closed

Add DataClass Hydra Support #40

SeanNaren opened this issue Jan 13, 2021 · 5 comments
Assignees
Labels
API enhancement New feature or request refactor & code health wontfix This will not be worked on

Comments

@SeanNaren
Copy link
Contributor

SeanNaren commented Jan 13, 2021

Related #37

The final piece is to add automatic defaults + type checking for dataclasses.

Two options:

  • Via the ConfigStore: https://hydra.cc/docs/next/tutorials/structured_config/schema/ which would need to store all configs that we'd like to type check/duck-type via Hydra. The object remains a DictConfig (duck-typing)
  • Carlos Idea: provide a _name_ within the config to automatically convert to a specific data-class, go through the tree recursively

Choice 1 is closer to Hydra, Choice 2 is better because we actually pass data-classes around.

@SeanNaren SeanNaren added bug / fix Something isn't working help wanted Extra attention is needed Priority P0 labels Jan 13, 2021
@SeanNaren SeanNaren added this to the 1.0 milestone Jan 13, 2021
@carmocca carmocca added API enhancement New feature or request refactor & code health and removed Priority P0 bug / fix Something isn't working help wanted Extra attention is needed labels Jan 19, 2021
@carmocca
Copy link
Contributor

carmocca commented Jan 25, 2021

Choice 2 recap:

  • You cant mix DictConfigs and dataclasses. The later will be converted: Setting non-dictconfig attributes omry/omegaconf#480
  • This means we cant use any DictConfigs. this forces to either use all dataclasses or introducing a third structure like argparse.Namespace. We chose the first option
  • There is no such thing as a dataclass that allows **kwargs. I hacked one myself but there is no easy way to set its types.
  • dataclass.fields() does not return non-typed fields. This in turn makes OmegaConf.structured not work: OmegaConf.structured does not parse dataclass attributes which are not typed omry/omegaconf#489
  • Even if you get all of this to work, instantiate(dataclass) will convert it back to a DictConfig so we didn't achieve anything.
  • Also, instantiate does not support Union types, so forget about adding them to your dataclasses.

So looks like we'll have to marry DictConfig or scrap Hydra...

@carmocca carmocca assigned SeanNaren and unassigned carmocca Jan 25, 2021
@SeanNaren
Copy link
Contributor Author

I've seen a few repos that have Hydra in place: https://github.com/facebookresearch/mmf

That might be a good place to have a look as to how things have been integrated there as well

@edenlightning edenlightning removed this from the 0.1 milestone Feb 22, 2021
@stale
Copy link

stale bot commented Apr 24, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added wontfix This will not be worked on and removed wontfix This will not be worked on labels Apr 24, 2021
@omry
Copy link

omry commented Jun 4, 2021

OmegaConf 2.1 can now convert the config to real dataclasses:

https://omegaconf.readthedocs.io/en/latest/usage.html#omegaconf-to-object

@stale
Copy link

stale bot commented Aug 3, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Aug 3, 2021
@stale stale bot closed this as completed Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API enhancement New feature or request refactor & code health wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants