Remove input kwargs passing to datamodules for specific arguments about lightning-transformers HOT 3 CLOSED

If we want to be explicit but at the same time be concise, we could take dataclasses as arguments, for example (from my project):

https://github.com/carmocca/PyLaia/blob/b85f5e9f3b9ded7ed20173b2759cbca65dbde199/laia/common/arguments.py#L212-L233

which is passed to the LightningModule (HTREngineModule):

https://github.com/carmocca/PyLaia/blob/b85f5e9f3b9ded7ed20173b2759cbca65dbde199/laia/scripts/htr/train_ctc.py#L68

and then used to configure_optimizers:

https://github.com/carmocca/PyLaia/blob/b85f5e9f3b9ded7ed20173b2759cbca65dbde199/laia/engine/engine_module.py#L41-L80

What do you think about that? The only disadvantage is having to create the dataclass. We could also allow passing a dict if users don't want to bother with it

from lightning-transformers.

Great suggestion, I think data classes can be extremely useful here! Let me hack something out on the PR branch and see how it plays out, Hydra has good integration with dataclasses which should make this nice.

from lightning-transformers.

So I gave it a shot (and probably spent more time than I should've :D) and turns out it isn't as simple as I thought :( from the dataclass perspective it's fine and I've put some example of how the dataclasses can look: https://github.com/PyTorchLightning/lightning-transformers/tree/draft/dataclasses

However it opens up a can of worms with Hydra. Hydras' interface for dataclasses is quite intrusive even in the minimal example. I've worked with this API before which worked fine for when we didn't expect the need for instantiation.

We would need to add the configs to the ConfigStore (which currently isn't many) but in my eyes looks a lot like boilerplate and could grow pretty large especially if a user wants to add their own. We should just be able to do everything we want via the conf files + adding our own dataset code. In addition, figuring how to instantiate an object + config took a lot of thought; more than a user of Hydra should need.

After weighing the positives/negatives it's best we stick with the proposed solution in #20, which is clear enough for the user to know what's happening.

I'll try get @tchaton's views on this as well on the Hydra side!

from lightning-transformers.