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

Recursive Defaults List support #1044

Merged
merged 1 commit into from
Oct 30, 2020
Merged

Recursive Defaults List support #1044

merged 1 commit into from
Oct 30, 2020

Conversation

omry
Copy link
Collaborator

@omry omry commented Oct 7, 2020

Closes #171
Closes #326
Closes #1080
Closes #1089

  • Add _self_ support (Ability to specify composition order of primary config file #326)
  • Support recursive defaults (with all the existing features, deletion, package rename, overriding)
  • Clean new TODOs in the code
  • Deprecated - group: null deletion form from defaults list in favor of ~group and ~group=value.
  • Ensure no performance regression (will likely require caching of loaded configs because the new system does two passes.
  • Include information about actually used defaults list in the generated Hydra config node.
  • Consider changing default for unspecified _self_ to be last in the list and not first as it is now
    • Decided against. While this is arguably more intuitive for config files it's less intuitive for dataclasses with defaults list.
  • Document recursive defaults (probably warrants a dedicated page just for the defaults list).
  • Document _self_
  • Deprecated old way of using interpolation in the defaults list. specifically the pattern in specializing configs will be removed in the future in favor of the simpler but more magical new style. Old style is still supported in 1.1 but will be removed in 1.2.

This:

defaults:
  - dataset: imagenet
  - model: alexnet
  - dataset_model: ${defaults.0.dataset}_${defaults.1.model}
    optional: true

Will become:

defaults:
  - dataset: imagenet
  - model: alexnet
  - dataset_model: ${dataset}_${model}`
    optional: true

@facebook-github-bot 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 7, 2020
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Oct 9, 2020

This pull request introduces 1 alert when merging 01379f3 into 6374fa2 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Oct 13, 2020

This pull request introduces 1 alert when merging 5a1f9f2 into 1503d1d - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Oct 20, 2020

This pull request introduces 1 alert when merging d5b494a into 8adcee0 - view on LGTM.com

new alerts:

  • 1 for Unreachable code

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Oct 22, 2020

This pull request introduces 1 alert when merging 878a458 into 8adcee0 - view on LGTM.com

new alerts:

  • 1 for Unreachable code

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Oct 22, 2020

This pull request introduces 1 alert when merging a708283 into 3775a29 - view on LGTM.com

new alerts:

  • 1 for Unreachable code

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Oct 22, 2020

This pull request introduces 1 alert when merging dd5caaa into 3775a29 - view on LGTM.com

new alerts:

  • 1 for Unreachable code

Core tests are passing
@omry omry changed the title Recursive defaults - WIP Recursive defaults support Oct 30, 2020
@omry omry changed the title Recursive defaults support Recursive Defaults List support Oct 30, 2020
@omry omry merged commit f82b168 into master Oct 30, 2020
@omry omry deleted the 171-recursive-defaults branch October 30, 2020 00:48
@nmichlo
Copy link

nmichlo commented Nov 25, 2020

Does this mean that the new style will also support nested variables for specialization? Or rather would the following work if I were to extend your last example?

defaults:
  - dataset: imagenet
  - model: alexnet
  - data_wrapper: ${dataset.data_type}_${model.data_input_mode}

This would be an extremely useful pattern, I am currently lacking this feature in an existing project, but just wanted to confirm before submitting a feature request.

@omry
Copy link
Collaborator Author

omry commented Nov 26, 2020

No. the defaults list will never be able to use config variables in interpolation.
The defaults list is used to compose final config - it obviously cannot access things in the config that is not there yet to determine how to compose the same config.

@omry
Copy link
Collaborator Author

omry commented Jan 1, 2021

FYI: #1170 re-implements this and will be merged soon.
There are some major changes relative to this implementation there.

pixelb added a commit to pixelb/hydra that referenced this pull request May 11, 2022
(when the version_base is >= 1.2).

The old ${defaults.N.name} format was deprecated,
and slated for removal in 1.2.

Follows on from the support of the newer format in PR facebookresearch#1044

Addresses issue facebookresearch#1891
pixelb added a commit that referenced this pull request May 11, 2022
(when the version_base is >= 1.2).

The old ${defaults.N.name} format was deprecated,
and slated for removal in 1.2.

Follows on from the support of the newer format in PR #1044

Addresses issue #1891
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
3 participants