-
-
Notifications
You must be signed in to change notification settings - Fork 615
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
Enable --cfg=job --resolve with ${hydra:...} resolver #1696
Conversation
This pull request introduces 1 alert when merging ecaba57 into 8fa67de - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice. See comment.
hydra/_internal/hydra.py
Outdated
cfg = self.compose_config( | ||
config_name=config_name, | ||
overrides=overrides, | ||
run_mode=RunMode.RUN, | ||
with_log_configuration=with_log_configuration, | ||
) | ||
HydraConfig.instance().set_config(cfg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer not to introduce a side effect to a getter function.
Can the caller do this instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I inlined the _get_cfg
method in 6828fcc.
hydra/_internal/hydra.py
Outdated
with read_write(cfg.hydra): | ||
OmegaConf.resolve(ret) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unrelated. What is the motivation for this? (Is there a missing test?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The HydraConfig.set_config
method sets a readonly
flag on cfg.hydra
, so OmegaConf.resolve
fails unless the flag is turned off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could unset the readonly flag in show_cfg and show_help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 57452d5. I only needed to unset the flag in show_cfg
. In show_help
, a readonly flag is no problem because we are using OmegaConf.to_yaml(..., resolve=...)
instead of OmegaConf.resolve(...)
.
This pull request introduces 1 alert when merging 6828fcc into 437e20d - view on LGTM.com new alerts:
|
hydra/_internal/hydra.py
Outdated
with read_write(cfg.hydra): | ||
OmegaConf.resolve(ret) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could unset the readonly flag in show_cfg and show_help.
news/1681.bugfix
Outdated
@@ -0,0 +1 @@ | |||
Fix the --cfg=job --resolve flags so that the ${hydra:...} resolver now prints properly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes --help as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I updated the news fragment in 751653f.
@hydra.main(config_path=".", config_name="config") | ||
def my_app(cfg: DictConfig) -> None: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you reuse simple_app here?
You can place the config next to it or somewhere else and override the used config like:
python tests/test_apps/simple_app/my_app.py --config-name=config --config-dir=tests/test_apps/app_with_cfg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 1d7bdd2.
tests/test_hydra.py
Outdated
"examples/tutorials/basic/your_first_hydra_app/2_config_file/my_app.py", | ||
["--help", "--resolve"], | ||
["hydra.help.template=$CONFIG", "db.user=${hydra:job.name}"], | ||
dedent( | ||
"""\ | ||
db: | ||
driver: mysql | ||
user: my_app | ||
password: secret | ||
""" | ||
), | ||
id="overriding_help_template:$CONFIG,resolve_interp_to_hydra_config", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally speaking, avoid testing with the tutorial apps except in tests/test_examples. (I know the code is not being consistent about it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made the change in 27052cf.
Co-authored-by: Omry Yadan <omry@fb.com>
This pull request introduces 1 alert when merging 1b4af22 into 437e20d - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 57452d5 into 437e20d - view on LGTM.com new alerts:
|
hydra/_internal/hydra.py
Outdated
def sanitize_hydra_cfg(cfg: DictConfig) -> None: | ||
""" | ||
Mutate the input cfg: | ||
- delete all toplevel keys except "hydra". | ||
- delete `hydra.help` and `hydra.hydra_help`. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3c87309 is a refactoring:
get_sanitized_hydra_cfg
->sanitize_hydra_cfg
get_sanitized_cfg
->sanitize_cfg
Before, these methods were making a deepcopy in some branches, and not making a deepcopy in other branches.
Now, they never make a deepcopy (i.e. they mutate their input directly). This refactoring should resolve the LGTM alerts that are posted in the PR's main thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like get_sanitized_hydra_cfg was always copying and get_sanitized_cfg was composing (so no need to copy).
Not sure what alerts you are referring to.
def get_sanitized_hydra_cfg(src_cfg: DictConfig) -> DictConfig:
cfg = copy.deepcopy(src_cfg)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…uts" This reverts commit 3c87309.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good.
This PR closes #1681, so that resolved references to
HydraConfig
can appearin the output from the
--cfg=job
flag. It is now possible to callpython my_app.py --cfg job --resolve
when the user config contains the
${hydra:...}
resolver.To implement this, several changes are made to the
hydra._internal.hydra.Hydra
class:Hydra._get_cfg
method, we now callHydraConfig.instance().set_config(cfg)
on the composedcfg
object,enabling the
${hydra:...}
resolver.Hydra._get_cfg
method would delete thecfg["hydra"]
package immediately. We now delay the deletion until after
OmegaConf.resolve
can be called. This is done by factoring out aHydra.get_sanitized_cfg
method.HydraConfig.set_config
method sets a'readonly'
flag, it isnow necessary to use a
read_write
/flag_override
context manager whencalling
OmegaConf.resolve
and when deleting thecfg["hydra"]
package.HydraConfig.instance().set_config(cfg)
in theHydra.app_help
method. This allows forpython my_app.py --help --resolve
to work even if the help template references the user's
$CONFIG
and the${hydra:...}
resolver appears in that user config.