Should we get rid of Hydra?

[SeanNaren]

Motivation

This is motivated by recent development of LightningCLI which IMO is simpler, and better supported primarily because it exists within Lightning.

Over time I've noticed that due to the changes upstream in Lightning, hydra default configs are going out of sync every so often.

I also notice that there are fundamental issues such as this #149 where the confs exist within the package. We assume that the user will always clone the repo and make their modifications there. I would be curious if this works for users currently.

Pitch

Remove Hydra, treating this library more as a set of components people can import into their own scripts. If we'd like to keep the ability to run training and testing from the library, we should use the LightningCLI instead of Hydra.

Alternatives

Keep as is :)

cc @mathemusician @Borda @carmocca

[carmocca]

Playing devil's advocate:

Over time I've noticed that due to the changes upstream in Lightning, hydra default configs are going out of sync every so often.

The CLI suffers from exactly this issue too. There's 2 proposals in jsonargparse to improve this a bit but they haven't been worked on at the moment:

omni-us/jsonargparse#79
omni-us/jsonargparse#91

I also notice that there are fundamental issues such as this #149 where the confs exist within the package. We assume that the user will always clone the repo and make their modifications there.

The CLI would also have this problem if we wanted to distribute its config files. But I guess we wouldn't need to as much if we made the switch.

treating this library more as a set of components people can import into their own scripts.

This is good regardless of the parsing library used. The choice of Hydra or the LightningCLI will come down to what the users are doing and how.

[mathemusician]

@SeanNaren @Borda Could you give a quick example of how people would use this library without hydra? If we can come up with a list of use cases where this would make the life of future users easier/simpler, then it would be a lot easier moving this issue forward.

When I started learning about this library, Hydra was the hardest thing to learn since it had the least documentation. Getting rid of it may increase usability, because it would be one less thing to learn. I had initially thought Hydra would make things easier, but it was only easier after learning about the entire library (which took me a month). :/ But that's probably because I'm a slow learner.

I propose an experiment, albeit, an expensive one. We replace Hydra with LightningCLI, and if more people use it, we'll stick with it. If not, we can always go back :0 Converting this library into a set of components may be challenging but worthwhile. It might even be merged into Bolts.

[SeanNaren]

Thank you both for your comments!

Addressing @carmocca comments:

The CLI suffers from exactly this issue too. There's 2 proposals in jsonargparse to improve this a bit but they haven't been worked on at the moment:

I'm unsure how these are related, it seems the second one is about trying to reproduce a saved config that was run on an earlier version of Lightning? I think this arguably could be expected behaviour. For Lightning-transformers specifically, I meant the fact that we rely on hardcoded parameters within the Trainer config. This has been outdated a few times as Lightning deprecates parameters/introduces parameters meaning users couldn't run the code out of the box unless they changed the configs manually.

This is good regardless of the parsing library used. The choice of Hydra or the LightningCLI will come down to what the users are doing and how.

Agreed, and currently this is supported (props to @carmocca who ensured that this remained). It can be seen from the test here.

The objective of this issue is to determine if we believe that making this experience a first class citizen would be beneficial for users and reduce complication by allowing users to pick what they'd like, defaulting to LightningCLI as it's the simpler of the two.

Addressing @mathemusician comments:

@SeanNaren @Borda Could you give a quick example of how people would use this library without hydra? If we can come up with a list of use cases where this would make the life of future users easier/simpler, then it would be a lot easier moving this issue forward.

The test here gives us a good sense, however still contains some ugliness due to Hydra. I imagine:

tokenizer = AutoTokenizer.from_pretrained(backbone="patrickvonplaten/t5-tiny-random")
model = TranslationTransformer(
    backbone="patrickvonplaten/t5-tiny-random"
)
dm = TranslationDataModule(
    batch_size=1,
    dataset_name="wmt16",
    dataset_config_name="ro-en",
    source_language="en",
    target_language="ro",
)
trainer = pl.Trainer()

trainer.fit(model, dm)

In this sense, we offer just LightningModules and LightningDataModules for the individuals tasks. the trainer can then be swapped out for the LightningCLI. Making this change also removes some of the boilerplate around instantiation etc. If the user wants to swap the optimizer/scheduler, they can do so via overriding configure_optimizers.

Personally I'm unsure which direction is better. I believe that simplicity of a library is important, and building frameworks rarely provide long term sustainability unless supported well (and the support for Lightning Transformers has been wishwashy, mostly my fault).

I like your idea of trying it out, seeing how the code looks like and then making a consensus whether to swap. But right now I think it would be good to ask a few more people to pitch in!

[carmocca]

I don't want to sidetrack this conversation but to clarify:

I meant the fact that we rely on hardcoded parameters within the Trainer config. This has been outdated a few times as Lightning deprecates parameters/introduces parameters meaning users couldn't run the code out of the box unless they changed the configs manually.

It's the same with the CLI, if you want to save the config inside the repository.
python script.py --print_config > your_config.yaml will include all the current arguments for the trainer etc, if they are removed, the config will also become stale.
omni-us/jsonargparse#79 helps by allowing us to specify metadata for this config, most importantly the version that generated it. This could be parsed later to improve the UX when the versions do not match.
omni-us/jsonargparse#91 helps by printing only the arguments that have been customized, which reduces the chances of it breaking due to changes in other non-changed arguments.

Now, back to the main discussion.

I'd say, pick a few common or popular use cases, then we try implementing them with both options and see which is most natural/intuitive/simple, etc. This will also show which of the core components are inflexible.

[SeanNaren]

Thanks for clarifying @carmocca!

Personally would like to give this is a go. I much prefer simpler packages, and I feel we could cut down complexity for elegancy and sustainability :)

[ElderWanng]

Hey guys, I'm considering writing a tiny demo combining transformers with PL CLI. The Doc of Cli didn't explain too much about model inheriting. E.g. I can't use a config Dict anymore and have to write all the arguments in an explicit way. When it comes to multi-level inheriting, I got to write all the parameters manually. This is against simplicity. So is there any "best practice" or working-around of this problem?

[SeanNaren]

hey @ElderWanng super late response.

Could you have a look at the latest code? Things have changed drastically, moving towards a class based approach :)

[SeanNaren]

This is complete! For the next release, we'll be moving to a PyTorch Lightning focused library.