Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-think how configs are handled in train.py #227

Closed
calebrob6 opened this issue Nov 7, 2021 · 2 comments · Fixed by #352
Closed

Re-think how configs are handled in train.py #227

calebrob6 opened this issue Nov 7, 2021 · 2 comments · Fixed by #352
Labels
trainers PyTorch Lightning trainers
Milestone

Comments

@calebrob6
Copy link
Member

Currently configuration to train.py is handled with OmegaConf. This made more sense when the tasks (and accompanying trainer code) were fragmented, as we could easily define per-task configuration. Now that the trainer code that we would like to include in base TorchGeo are being generalized into things like ClassificationTask and SemanticSegmentationTask and it is clear that more complicated training configurations won't be supported by torchgeo proper, it might make sense to pull out the OmegaConf part, and go with a more simple argparse based approach. Bonus: this would also allow us to get rid of a dependency. I'm not sure how exactly the argparse approach would work in all cases but it is worth more thought!

Lightning has a few pieces of docs that can help with this:

Whatever we settle on here should definitely still allow passing arguments via a YAML config file. This allows reproducible benchmark experiment configurations to be saved in source control.

@adamjstewart adamjstewart added the trainers PyTorch Lightning trainers label Nov 7, 2021
@adamjstewart
Copy link
Collaborator

In #226 I did a pretty impressive job of abusing the configs for the sake of testing. Once the release is finished, we'll want to go back through this and clean things up. First order of business is to create a tests/conf directory to hold testing-specific config settings so that we don't need to abuse conf/task_defaults anymore. Second order of business is to try to reduce some of the duplication between conf/*.yaml and conf/task_defaults/*.yaml. Third order of business is to try and improve documentation/CLI so it's easier to tell what settings are available. I also want to see if there's some kind of validation thing we could use to ensure that all settings in a config file are actually valid.

@adamjstewart adamjstewart added this to the 0.2.0 milestone Nov 20, 2021
@adamjstewart adamjstewart modified the milestones: 0.2.0, 0.3.0 Jan 1, 2022
@calebrob6
Copy link
Member Author

In #286 we changed the number of classes in the task_default config files to make the tests pass. As a side effect, this breaks training runs where the config files don't have the correct number of classes specified. Users shouldn't need to figure out / input the correct number of classes for a dataset in a config file in order to run train.py.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
trainers PyTorch Lightning trainers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants