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 support for deprecated hydra override syntax #2056

Conversation

Jasha10
Copy link
Collaborator

@Jasha10 Jasha10 commented Feb 25, 2022

This PR removes deprecated support for overriding hydra config groups without use of the keyword override (see this deprecation notice).

Removing this deprecated behavior enables the use of custom subgroups of the top-level hydra config.

Example:

Suppose we have a directory tree like this:

├── conf
│   └── hydra
│       └── run
│           └── my_run.yaml
└── my_app.py

Let's say my_app.py is a minimal Hydra app:

# my_app.py
import hydra

@hydra.main(config_path="conf", config_name=None)
def app(cfg):
    print(cfg)

if __name__ == "__main__":
    app()

Before this PR, it is impossible to add hydra/run: my_run to the defaults list (since hydra/run does not appear in the defaults list of HydraConf).

$ # Before this PR:
$ python my_app.py hydra.job.chdir=true +hydra/run=my_run
/Users/jasha10/hydra.git/hydra/_internal/defaults_list.py:413: UserWarning: In _dummy_empty_config_: Invalid overriding of hydra/run:
Default list overrides requires 'override' keyword.
See https://hydra.cc/docs/next/upgrades/1.0_to_1.1/defaults_list_override for more information.

  deprecation_warning(msg)
In '_dummy_empty_config_': Could not override 'hydra/run'. No match in the defaults list.

Set the environment variable HYDRA_FULL_ERROR=1 for a complete stack trace.
$ # After this PR:
$ python my_app.py hydra.job.chdir=true +hydra/run=my_run
{}

This enabled use-case is relevant to discussion #2051.

@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 Feb 25, 2022
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Feb 25, 2022

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

new alerts:

  • 1 for Unused import

@Jasha10 Jasha10 force-pushed the remove-deprecated-defaults-list-overrides-behavior branch 2 times, most recently from dab9948 to 4d6e5f7 Compare February 25, 2022 03:57
@jieru-hu
Copy link
Contributor

thanks @Jasha10
what would users see if they still have the old style override (- hydra/job_logging: colorlog) in their defaults list?

@Jasha10
Copy link
Collaborator Author

Jasha10 commented Feb 25, 2022

Using the old style override:

defaults:
  - hydra/job_logging: colorlog
  - hydra/hydra_logging: colorlog
$ python my_app.py hydra.job.chdir=true
Multiple values for hydra/job_logging. To override a value use 'override hydra/job_logging: colorlog'

Set the environment variable HYDRA_FULL_ERROR=1 for a complete stack trace.

@Jasha10 Jasha10 marked this pull request as draft March 1, 2022 17:58
@Jasha10
Copy link
Collaborator Author

Jasha10 commented Mar 1, 2022

I'm marking this PR as a draft until the new Hydra base_version functionality is in place.

@Jasha10 Jasha10 force-pushed the remove-deprecated-defaults-list-overrides-behavior branch from 8d9abc1 to 91bc0c5 Compare April 21, 2022 16:09
@Jasha10 Jasha10 marked this pull request as ready for review April 21, 2022 16:24
@Jasha10 Jasha10 added this to the Hydra 1.2.0 milestone Apr 21, 2022
@Jasha10 Jasha10 requested a review from jieru-hu April 21, 2022 16:34
@Jasha10 Jasha10 merged commit e8a03a7 into facebookresearch:main Apr 22, 2022
@Jasha10 Jasha10 deleted the remove-deprecated-defaults-list-overrides-behavior branch April 22, 2022 16:45
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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants