TODO list for "replace Hparams by init args" PR

[awaelchli]

🚀 TODO: Follow up work on module arguments rework in #1896

• 1. (docs) Make clear the multiple ways args can and cannot be passed in.
  Example:

   class LitModel(LightningModule):
      def __init__(self, arg1, arg2):
       ...
   Trainer.add_argparse_args(parser)
   LitModel.add_model_specific_args(parser)
   LitModel(parser.parse_args())  # this will fail

  This won't work since the list of arguments in constructor is a fixed size.
  We can fix it in two ways:

  1. Add **kwargs to the init signature to catch any unnecessary args (not good design but works)
  2. Split the parsers to separate model args from Trainer args

• 2. (docs) make it clear which types we save to the checkpoints and which not (nn.Module for example). The name "module_arguments" maybe misleading to believe all args are saved.

• 3. Some old code was left commented, including tests, as mentioned by @yukw777

• 4. (tests) The model checkpointing has changed, we should thoroughly test that the correct args are loaded.

• 5. (tests) Test case for positional args

• 6. (bugfix) Fix for when super() is not called or called after other local vars were added, e.g.,

   class LitModel(LightningModule):
      def __init__(self, arg1, arg2):
          my_local_var = 2
          super().__init__()
          # module_arguments now contains "my_local_var"
   
   LitModel.load_from_checkpoint(...)  # this fails
   # TypeError: __init__ got an unexpected argument "my_local_var"

  We obviously don't want any local vars other than the arguments in the checkpoint.

• 7. (bugfix) In Python we are not forced to call the instance "self", this is currently hardcoded and leads to:

   class LitModel(LightningModule):
      def __init__(obj, arg1, arg2):
          obj.arg1 = arg1
          super().__init__()
          # module_arguments will contain LitModel() itself

  same applies to the conventional naming of "*args" and "**kwargs"

• 8. (tests) make sure the LRfinder still works as expected by passing in the suggested learning rate as argument (fixed in Bugfix: Lr finder and hparams compatibility #2821 )

• 9. (enhancement) @festeh wants to add support for dataclasses

• 10. (bugfix) some of the examples are broken because of the problem mentioned in 1.

• 11. (test) multiple inheritance

• 12. Should error or warn when self.auto_collect_arguments() is called somewhere other than in init. A specific use case that is currently not working is Crash trying to construct module_arguments when module is created in a method #1976

Feel free to add additional bullet points I missed :)

[justusschock]

We should also make sure, that the current hparams will always be supported. There are definitely usecases where hparams are not suitable.

[Borda]

We should also make sure, that the current hparams will always be supported. There are definitely usecases where hparams are not suitable.

they are as Namespace and dict are in allowed primitives

[justusschock]

@Borda yes, but to make sure, I'd prefer to have an explicit test for this :)

Since we should really take care of backwards compatibility.

[Borda]

@Borda yes, but to make sure, I'd prefer to have an explicit test for this :)
Since we should really take care of backwards compatibility.

Sure, agree, mind draw the test in PR and I will finish it / ensure the compatibility =)

[HansBambel]

 class LitModel(LightningModule):
    def __init__(self, arg1, arg2):
     ...
 Trainer.add_argparse_args(parser)
 LitModel(parser.pase_args())  # this will fail

@awaelchli Just for clarification: this will not fail because you have a typo in parser.pase_args(), but because the call is not supported, right?

[awaelchli]

yes exactly, it will fail because the argparser has many more args than just arg1, arg2.
I will fix the typo.

[Borda]

@awaelchli let's update the list with respect to what has been done...
@edenlightning mind help?

[edenlightning]

@awaelchli whats left here?

[awaelchli]

I think most of the points are outdated, much has changed. I think we can close it and track any remaining issues via reported bugs. Although I think testing of the "save_hyperparameters" feature could be more thorough in general (bullet points 5., 8., 11)