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

Improve error message for merging from defaults list #1697

Closed
Jasha10 opened this issue Jun 25, 2021 · 1 comment · Fixed by #1824
Closed

Improve error message for merging from defaults list #1697

Jasha10 opened this issue Jun 25, 2021 · 1 comment · Fixed by #1824
Assignees
Labels
enhancement Enhanvement request

Comments

@Jasha10
Copy link
Collaborator

Jasha10 commented Jun 25, 2021

When composing a config from a defaults list, merge errors can occur if the source config (the selected default) does not fit the schema of the target.

When such an error happens, it would be nice if the error message would say which entry in the defaults list is responsible for the merge error. This would help with debugging when an exception occurs.

Example

Here is an example where the error message could be improved.
A python file:

# my_app.py
from dataclasses import dataclass
import hydra

@dataclass
class MainConf:
    data: dict[str, int]

cs = hydra.core.config_store.ConfigStore.instance()
cs.store(name="main_schema", node=MainConf, provider=__file__)

@hydra.main(config_path="conf", config_name="main")
def main(cfg) -> None:
    pass

main()

Two yaml files in the conf/ directory:

# conf/main.yaml
defaults:
  - main_schema
  - my_data
# conf/my_data.yaml
foo: 123

And here is what happens when you run Hydra:

$ python my_app.py

Key 'foo' not in 'MainConf'
    full_key: foo
    object_type=MainConf

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

The error message should contain the word "my_data" somewhere, so that the user can know which entry from the defaults list caused the problem.
The error is resolved if the main.yaml defaults list uses - my_data@data instead of - my_data.

Implementation:

In the hydra._internal.config_loader_impl.ConfigLoaderImpl class, there is a _compose_config_from_defaults_list method that has a for loop that looks like this:

            for default in defaults:
                loaded = self._load_single_config(default=default, repo=repo)
                try:
                    cfg.merge_with(loaded.config)
                except ValidationError as e:
                    raise ConfigCompositionException(
                        f"In '{default.config_path}': Validation error while composing config:\n{e}"
                    ).with_traceback(sys.exc_info()[2])

To improve the error message emitted, it should be sufficient to add another except block:

                except ConfigKeyError as e:
                    raise ConfigCompositionException(
                        f"Config key error while merging default '{default.config_path}':\n{e}"
                    ).with_traceback(sys.exc_info()[2])

After adding this except block, the printed error message looks like this:

$ python my_app.py
Config key error while merging default 'my_data':
Key 'foo' not in 'MainConf'
    full_key: foo
    object_type=MainConf

Set the environment variable HYDRA_FULL_ERROR=1 for a complete stack trace.
@Jasha10 Jasha10 added the enhancement Enhanvement request label Jun 25, 2021
@Jasha10 Jasha10 self-assigned this Jun 25, 2021
@omry
Copy link
Collaborator

omry commented Jun 28, 2021

Sounds good, except you should just catch OmegaConfBaseException instead of adding another catch clause.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhanvement request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants