refactor DefaultTrainer.from_checkpoint

[FynnBe]

No description provided.

[constantinpape]

Only had a brief look at the changes, I think this should work in principle.
But tests are still failing:

Traceback (most recent call last):
  File "/home/runner/work/torch-em/torch-em/test/trainer/test_default_trainer.py", line 81, in test_from_checkpoint
    trainer2 = DefaultTrainer.from_checkpoint(
  File "/home/runner/work/torch-em/torch-em/torch_em/trainer/default_trainer.py", line 195, in from_checkpoint
    trainer_kwargs = cls._get_trainer_kwargs(load)
  File "/home/runner/work/torch-em/torch-em/torch_em/trainer/default_trainer.py", line 175, in _get_trainer_kwargs
    train_loader=load("train_loader"),
  File "/home/runner/work/torch-em/torch-em/torch_em/trainer/default_trainer.py", line 126, in __call__
    return self.load_loader(name)
  File "/home/runner/work/torch-em/torch-em/torch_em/trainer/default_trainer.py", line 154, in load_loader
    ds = self.init_data[f"{name}_dataset"]
KeyError: 'train_loader_dataset'

Please make sure that they pass.

[constantinpape]

Looks good overall, but two minor comments left to address.

[constantinpape]

Is there a particular reason you're not using **dynamic_kwargs here?

[FynnBe]

yes, **dynamic_kwargs would not allow to optional (or only_class) as a kwarg

[constantinpape]

Maybe I am missing something, but I don't see why this is the case. As far as I can see this is perfectly valid python:

def my_func(
    kwarg_name, *args,
    optional=False, only_class=False,
    **kwargs
):
    print(optional)

my_func("x", "y", optional=True, blub="xyz")

(prints True)

[constantinpape]

What exactly is name here? I thought it was the name of the trainer, i.e. the checkpoint name. But looks like it's something else.

[constantinpape]

Ok, I understand now that this is the name of the kwarg. But this should be explained a bit better, it's somehow not immediately obvious. I think it would be best if you can just add the "normal usage" in the doc string, i.e. how the deserializer is normally called for a kwarg.

[FynnBe]

good idea. I suppose we can additionally rename name to kwarg_name or attribute?

[constantinpape]

Yes, let's go with kwarg_name

[constantinpape]

I still don't see why we can't use **kwargs in __call__. Looks good otherwise.

[constantinpape]

Maybe I am missing something, but I don't see why this is the case. As far as I can see this is perfectly valid python:

def my_func(
    kwarg_name, *args,
    optional=False, only_class=False,
    **kwargs
):
    print(optional)

my_func("x", "y", optional=True, blub="xyz")

(prints True)

[FynnBe]

I still don't see why we can't use **kwargs in __call__. Looks good otherwise.

This would fail in case someone needs a special dynamic_kwarg that happens to also be called optional...
It's an edge case, sure...
The updated, simplified version of this does not have this issue anymore: FynnBe#1
It builds on top of this PR, but changes the Deserializer a bit to be more analogous to the new Serializer, which is used in _build_init...

[constantinpape]

Ah ok, I see what you mean. Quite a corner case indeed, but good to catch it here.

The updated, simplified version of this does not have this issue anymore: FynnBe#1

Cool, I will have a look once you think it's good to go and create it as a PR here, for now I think the changes in this one are good to go and I'll merge.