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

Remove trainer args from conf/defaults.yaml #351

Merged
merged 4 commits into from
Jan 6, 2022

Conversation

ashnair1
Copy link
Collaborator

@ashnair1 ashnair1 commented Jan 2, 2022

Closes #347

@github-actions github-actions bot added the datamodules PyTorch Lightning datamodules label Jan 2, 2022
@github-actions github-actions bot removed the datamodules PyTorch Lightning datamodules label Jan 2, 2022
@ashnair1
Copy link
Collaborator Author

ashnair1 commented Jan 2, 2022

  1. While defaults.yaml pinned torchgeo to use newer versions of pytorch-lightning it was useful in documenting what values could be set. Now that it's being removed, users don't really have a reference to set up their training configs. For training args I guess they can refer https://pytorch-lightning.readthedocs.io/en/<version>/common/trainer.html#init (replace version with whatever version of pytorch-lightning they're using).

  2. The experiment level arguments (seed, output_dir, log_dir) must be set in all task yamls for training to run. I can add something like this to all the task_defaults configs.

program:  # These are experiment level arguments
  seed: 0
  experiment_name: blah
  overwrite: True
  output_dir: output
  data_dir: data
  log_dir: logs

Edit: only seed, output_dir and log_dir are required in above config

@ashnair1 ashnair1 marked this pull request as ready for review January 2, 2022 10:01
train.py Outdated Show resolved Hide resolved
@adamjstewart
Copy link
Collaborator

  1. Hmm, we may want to keep a defaults.yaml that only contains these defaults. @calebrob6 thoughts?

@ashnair1
Copy link
Collaborator Author

ashnair1 commented Jan 2, 2022

Keeping a defaults.yaml with just these defaults seem like a good idea since they're not related to any library. The issue in #347 was only with trainer args which was dependent on the version of PL.

Plus copying these settings to all configs seems unnecessary

@adamjstewart
Copy link
Collaborator

Some of those do actually make sense to override on a dataset-level. Like experiment_name and *_dir could be dataset-specific so all output doesn't end up in the same directory.

@adamjstewart
Copy link
Collaborator

Another option is to make all settings (not just the ones in defaults.yaml) optional using something like getattr.

@ashnair1
Copy link
Collaborator Author

ashnair1 commented Jan 2, 2022

Some of those do actually make sense to override on a dataset-level. Like experiment_name and *_dir could be dataset-specific so all output doesn't end up in the same directory.

Basically I was thinking to leave the first half of conf/defaults.yaml more or less as is. So this is all it would contain:

config_file: null  # This lets the user pass a config filename to load other arguments from

program:  # These are the arguments that define how the train.py script works
  seed: 0
  output_dir: output
  log_dir: logs
  overwrite: False

We only need to specify the seed, output_dir and log_dir because they're explicitly used in train.py. experiment_name would go in the specific task.yaml. I made a mistake including it before.

For further customisation of *_dir users would need to write a new train.py right?

@ashnair1 ashnair1 changed the title Remove conf/defaults.yaml Modify conf/defaults.yaml Jan 3, 2022
@ashnair1 ashnair1 changed the title Modify conf/defaults.yaml Remove trainer args from conf/defaults.yaml Jan 6, 2022
@adamjstewart adamjstewart merged commit f69bdaf into microsoft:main Jan 6, 2022
@ashnair1 ashnair1 deleted the trainer-defaults branch January 7, 2022 10:49
@adamjstewart adamjstewart modified the milestones: 0.3.0, 0.2.1 Mar 19, 2022
adamjstewart pushed a commit that referenced this pull request Mar 19, 2022
* Remove defaults.yaml

* Circumvent KeyError

* Bring back defaults.yaml

* Add newline
yichiac pushed a commit to yichiac/torchgeo that referenced this pull request Apr 29, 2023
* Remove defaults.yaml

* Circumvent KeyError

* Bring back defaults.yaml

* Add newline
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

detect_anomaly not supported for pytorch-lightning<1.5
2 participants