hydra-torch icon indicating copy to clipboard operation
hydra-torch copied to clipboard

[WIP] Intermediate Tutorial

Open romesco opened this issue 5 years ago • 2 comments

romesco avatar Oct 26 '20 22:10 romesco

@tkornuta-nvidia I'm testing the DataLoaderConf class - seems to be working fine!

What are the thoughts on supporting the Dataset class as well as MNIST from torchvision? I think we have yet to settle on how we expect to structure the repo when allowing classes from another project.

By the way, one other thing I've noticed:

Logic like this, IMO, doesn't really deserve to live in the main file:

 if use_cuda:
        cuda_kwargs = {"num_workers": 1, "pin_memory": True, "shuffle": True}
        train_kwargs.update(cuda_kwargs)
        test_kwargs.update(cuda_kwargs)

What would be really cool is to support a conditional config. I know that's not possible with hydra at the moment, but something like:

@dataclass
class MNISTConf:
    no_cuda: bool = False
    ...
    checkpoint_name: str = "unnamed.pt"
    data_train: DataLoaderConf = DataLoaderConf(
        shuffle = True,
        num_workers = 0 if {no_cuda} else 1,
        pin_memory = False
    )
    data_test: DataLoaderConf = DataLoaderConf(
        shuffle = False,
        num_workers = 0 if {no_cuda} else 1
    )
    model: MNISTNetConf = MNISTNetConf()
    ...
    )

romesco avatar Oct 30 '20 19:10 romesco

@tkornuta-nvidia I'm testing the DataLoaderConf class - seems to be working fine!

What are the thoughts on supporting the Dataset class as well as MNIST from torchvision? I think we have yet to settle on how we expect to structure the repo when allowing classes from another project.

By the way, one other thing I've noticed:

Logic like this, IMO, doesn't really deserve to live in the main file:

 if use_cuda:
        cuda_kwargs = {"num_workers": 1, "pin_memory": True, "shuffle": True}
        train_kwargs.update(cuda_kwargs)
        test_kwargs.update(cuda_kwargs)

Yeah, this is definitely not great. Maybe this will help, but it's a big hammer. There is probably a better way to model this for this case.

omry avatar Oct 31 '20 17:10 omry