Structured Configs with URL for torch.optim and torch.utils.data #18

tkornuta-nvidia · 2020-10-16T21:25:19Z

Summarizing, this PR introduces:

a separate confige.yamln configuration files for torch.optim
a separate configen.yaml configuration files for torch.utils.data ( [hydra-configs-torch] Configs for Datasets and Samplers #1 )
re-generated Structured Configs for modules from torch.optim
generated Structured Config for torch.utils.data.DataLoader class

This is the first try of using configen - in short: great job with this one, @omry, "meta-program or die!" ;)
Please note for generation of all Structured Configs I have used modified version of configen: (Hydra PR: facebookresearch/hydra#1082)

I assumed we will have different contributors for different modules. So to increase the modularity I have reorganized the source folder structure (source = folder containing specification of modules to be generated). Due to the current limitation of configen (hardcoded "configen.yaml" file) the proposed solution has the following directory structure:

 - source/
    - torch.optim/configen.yaml # contains specification of all torch.optim modules
    - torch.utils.data/configen.yaml # contains specification of torch.utils.data modules

Signed-off-by: Tomasz Kornuta <[email protected]>

romesco · 2020-10-17T02:34:42Z

@tkornuta-nvidia Are you envisioning we have many conf/configen-<module_specifc>.yaml files per project, rather than one monolithic config yaml per project? I'm not opposed, just wanna see if that's what you're proposing 🙂.

omry · 2020-10-17T07:10:47Z

as test, moved it to a different file - I assume we will have different contributors, hence different headers (e.g. copyright)

For now I think we should stick to a single file until we have a good reason to start splitting to config groups.
All contributors will use the same copyright: Facebook's. it's a requirement for all committed code (and anyone signing the CLA agrees to it).

Key 'init_config_dir' is not in struct
I am not sure the configen pip package is updated, did you install from Hydra master?
Take a look at the code and figure out if that needs to be there or not.

omry · 2020-10-17T07:12:37Z

@romesco:
The CI failures are because you pushed stuff that failed CI checks. Please fix it :).
We should only push stuff that passes all checks.

omry · 2020-10-17T07:34:29Z

also, I think we should get into the habit of always adding tests when we add new configs.

romesco · 2020-10-17T19:54:28Z

@romesco:
The CI failures are because you pushed stuff that failed CI checks. Please fix it :).
We should only push stuff that passes all checks.

Yes, sorry it was because of the reorganization of file structure. It's only a linting problem in the __init__.py within optim. Fixing that.

tkornuta-nvidia · 2020-10-19T16:39:10Z

@tkornuta-nvidia Are you envisioning we have many conf/configen-<module_specifc>.yaml files per project, rather than one monolithic config yaml per project? I'm not opposed, just wanna see if that's what you're proposing 🙂.

Yes, this is what I am proposing.

each file will be lighter, I think about having a file per pytorch module.
the header/licenses - I need to obey some rules, I guess other (external?) contributors might need to add something to their headers too...

tkornuta-nvidia · 2020-10-19T16:40:15Z

@romesco:
The CI failures are because you pushed stuff that failed CI checks. Please fix it :).
We should only push stuff that passes all checks.

ehehe, yeah, I know - still, quoting Shaggy" wasn't me" ;)

omry · 2020-10-19T17:40:48Z

@tkornuta-nvidia Are you envisioning we have many conf/configen-<module_specifc>.yaml files per project, rather than one monolithic config yaml per project? I'm not opposed, just wanna see if that's what you're proposing 🙂.

Yes, this is what I am proposing.
1. each file will be lighter, I think about having a file per pytorch module.

2. the header/licenses - I need to obey some rules, I guess other (external?) contributors might need to add something to their headers too...

I think we should use Hydra to compose configs for us much like we do otherwise.
configen is powered by Hydra, we can have a config group for project and do a sweep to generate configs for all projects.
There are also some questions are PyTorch version and Hydra version, which may be additional sweep dimensions eventually.

…ate configs with additional URL links Signed-off-by: Tomasz Kornuta <[email protected]>

Signed-off-by: Tomasz Kornuta <[email protected]>

…SPDX identifier Signed-off-by: Tomasz Kornuta <[email protected]>

Signed-off-by: Tomasz Kornuta <[email protected]>

romesco · 2020-10-20T21:26:48Z

sources/torch.optim/configen.yaml

@@ -1,9 +1,13 @@
+# Copyright (c) 2020, Facebook, Inc. and its affiliates. All Rights Reserved


Regarding the change from conf directory to sources directory, I think this is a good idea. I think the term conf/config/configen is very overloaded in this setting. (something I commented on a while back too). It made knowing where to look for things imo overly difficult until I got used to the lay of the land. I might even argue this should be propagated to the default dir for configen.

config/torch/optim/lr_scheduler.py

Signed-off-by: Tomasz Kornuta <[email protected]>

tkornuta-nvidia · 2020-10-20T21:49:45Z

config/torch/optim/lr_scheduler.py

@@ -66,12 +67,12 @@ class CosineAnnealingLRConf:
 class ReduceLROnPlateauConf:
    _target_: str = "torch.optim.lr_scheduler.ReduceLROnPlateau"
    optimizer: Any = MISSING
-    mode: str = 'min'


Hey @romesco pulled latest hydra changes and regenerated configs - ` -> "

omry

Let's hold on with this one until configen is ready to support this.

omry · 2020-10-20T23:07:04Z

config/torch/optim/adadelta.py

@@ -1,4 +1,5 @@
 # Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved
+# SPDX-License-Identifier: MIT


Please do not change the headers until we resolve the discussion about it.

omry · 2020-10-20T23:08:02Z

config/torch/optim/adadelta.py

@@ -13,6 +14,7 @@

 @dataclass
 class AdadeltaConf:
+


what are you changing the formatting of generated code?

omry · 2020-10-20T23:08:59Z

config/torch/optim/adagrad.py

+    """For more details on parameteres please refer to the original documentation:
+    https://pytorch.org/docs/stable/optim.html#torch.optim.Adagrad
+    """


Let's hold on with this change until we finalize the change to configen. This is not urgent and we can do it with configen once it's ready.

Initial version of dataloader

9d5decd

Signed-off-by: Tomasz Kornuta <[email protected]>

tkornuta-nvidia requested a review from omry October 16, 2020 21:25

tkornuta-nvidia added the enhancement New feature or request label Oct 16, 2020

added license to yaml

42c0da8

Signed-off-by: Tomasz Kornuta <[email protected]>

romesco self-requested a review October 17, 2020 02:30

tkornuta-nvidia added 3 commits October 20, 2020 13:22

fixed headers, reorganized folders, used modified configen to regener…

31bd189

…ate configs with additional URL links Signed-off-by: Tomasz Kornuta <[email protected]>

regenerated adadelta - with url

3554c17

Signed-off-by: Tomasz Kornuta <[email protected]>

regenerated files - with tiny fix in headers, only facebook header + …

ced1ecb

…SPDX identifier Signed-off-by: Tomasz Kornuta <[email protected]>

tkornuta-nvidia changed the title ~~Initial version of DataLoader~~ Structured Configs with URL for torch.optim and torch.utils.data Oct 20, 2020

tkornuta-nvidia marked this pull request as ready for review October 20, 2020 21:10

tkornuta-nvidia added 2 commits October 20, 2020 14:11

Merge branch 'master' of github.com:pytorch/hydra-torch into dev-utils

3fff4b7

regeneration after rebase

f596a5f

Signed-off-by: Tomasz Kornuta <[email protected]>

romesco reviewed Oct 20, 2020

View reviewed changes

tkornuta-nvidia added 2 commits October 20, 2020 14:40

reverted min -> 'min'

e074842

Signed-off-by: Tomasz Kornuta <[email protected]>

regenerated torch.optim with lastest + modifed configen

752fc1e

Signed-off-by: Tomasz Kornuta <[email protected]>

tkornuta-nvidia commented Oct 20, 2020

View reviewed changes

omry suggested changes Oct 20, 2020

View reviewed changes

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Structured Configs with URL for torch.optim and torch.utils.data #18

Structured Configs with URL for torch.optim and torch.utils.data #18

tkornuta-nvidia commented Oct 16, 2020 •

edited

Loading

romesco commented Oct 17, 2020

omry commented Oct 17, 2020

omry commented Oct 17, 2020

omry commented Oct 17, 2020

romesco commented Oct 17, 2020 •

edited

Loading

tkornuta-nvidia commented Oct 19, 2020

tkornuta-nvidia commented Oct 19, 2020 •

edited

Loading

omry commented Oct 19, 2020

romesco Oct 20, 2020

tkornuta-nvidia Oct 20, 2020

omry left a comment

omry Oct 20, 2020

omry Oct 20, 2020

omry Oct 20, 2020

		@@ -1,9 +1,13 @@
		# Copyright (c) 2020, Facebook, Inc. and its affiliates. All Rights Reserved

		@@ -1,4 +1,5 @@
		# Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved
		# SPDX-License-Identifier: MIT

Structured Configs with URL for torch.optim and torch.utils.data #18

Are you sure you want to change the base?

Structured Configs with URL for torch.optim and torch.utils.data #18

Conversation

tkornuta-nvidia commented Oct 16, 2020 • edited Loading

romesco commented Oct 17, 2020

omry commented Oct 17, 2020

omry commented Oct 17, 2020

omry commented Oct 17, 2020

romesco commented Oct 17, 2020 • edited Loading

tkornuta-nvidia commented Oct 19, 2020

tkornuta-nvidia commented Oct 19, 2020 • edited Loading

omry commented Oct 19, 2020

romesco Oct 20, 2020

Choose a reason for hiding this comment

tkornuta-nvidia Oct 20, 2020

Choose a reason for hiding this comment

omry left a comment

Choose a reason for hiding this comment

omry Oct 20, 2020

Choose a reason for hiding this comment

omry Oct 20, 2020

Choose a reason for hiding this comment

omry Oct 20, 2020

Choose a reason for hiding this comment

tkornuta-nvidia commented Oct 16, 2020 •

edited

Loading

romesco commented Oct 17, 2020 •

edited

Loading

tkornuta-nvidia commented Oct 19, 2020 •

edited

Loading