facebookresearch / hydra

Hydra is a framework for elegantly configuring complex applications
https://hydra.cc
MIT License
8.82k stars 636 forks source link

[Feature Request] check `DictConfig` to detect missed mandatory fields #399

Closed EgorBu closed 3 years ago

EgorBu commented 4 years ago

🚀 Feature Request

First of all - thanks for a great work!) I think a lot of people want to use hydra as a replacement for argparse. In my particular case, I want to have functionality to check that some mandatory arguments weren't provided.

Motivation

I want to detect problems as early as possible - and one way is to check that nothing is missed during configuration (at least, mandatory fields - argparse will throw exception if some arguments weren't provided)

Pitch

Describe the solution you'd like Something simple like this in DictConfig class probably - or in utils

def has_missed_mandatory_args(cfg: DictConfig) -> bool:
    """
    Check if some mandatory fields were not provided.
    :param cfg: config to check.
    :return: True if missed, False if not missed.
    """
    assert isinstance(cfg, DictConfig)
    for key in cfg:
        try:
            if isinstance(cfg[key], DictConfig):
                if has_missed_mandatory_args(cfg[key]):
                    # early stop
                    return True
        except MissingMandatoryValue:
            return True
    return False

Describe alternatives you've considered Implement it by myself - but I could miss some corner cases.

Are you willing to open a pull request? (See CONTRIBUTING)

Yes, I can make PR

omry commented 4 years ago

Hi @EgorBu, thanks for your kind words. I think using Hydra as an argparse replacement makes sense, but keep the composition in mind for when your config becomes complex enough (A lot of the power provided by Hydra kicks in once you start composing your config).

You can use OmegaConf.is_missing() as a supported API to determine if a field is missing. This is not recursive like your implementation but can simplify it.

I am not sure this belong in Hydra, if anything it might have a home next to OmegaConf.is_missing(), but I can imagine use cases for returning a list of missing keys too.

For now keep this implementation in your own project (you can add a few test cases, covering ListConfig as well and switching to OmegaConf.is_missing()).

I am closing because I would like additional support for this before adding it to either OmegaConf or Hydra (I did not personally felt the need for this so far). If more people express interest in this I can revisit.

Thanks for the feature request, please join the Hydra chat if you want to discuss this or other aspects of Hydra and OmegaConf.

greaber commented 4 years ago

A related idea I had was that in addition to the special ??? value, you could add a ???? value that would raise an error immediately if that value were not present. Maybe this does break down in some complex cases, but it seems like it could be nice in simple ones. Not sure though because I just started with hydra.

omry commented 4 years ago

Hi @greaber. Not present when?

This just sounds like a potential feature of Hydra that would trigger a failure if a composed config has missing values. this is something I can consider, but I do not see the point in introducing another kind of MISSING.

greaber commented 4 years ago

As I said, I'm not sure it is worth it. But I'm just adapting a model using a different config system that used a combination of a config file and some argparse options. And some of those options were mandatory, so argparse would just raise an error if you didn't set them. And that seemed to me like kind of a natural behavior and easier than having to manually check for missing mandatory arguments and raise an error (or else just waiting until the mandatory argument is accessed to raise an error, but in some cases that might not happen for a while).

omry commented 4 years ago

I can see use cases where it would make sense to permit a missing config option, for example if the application wants to manually compose additional things into the config at runtime. on the other hand, I also see the value in failing early in such cases.

I can revisit this for 1.1. A potential solution is to just introduce a new flag for @hydra.main() like permit_missing : bool = False, which by default fail early if there is a missing value in the config.

omry commented 4 years ago

I need to think this through though, so I may still end up abandoning this idea.

omry commented 3 years ago

Here is a function that can validate that recursively. You can use it. For now I am not planning on adding it to either Hydra or OmegaConf.

from typing import Any

from omegaconf import ListConfig, DictConfig, OmegaConf, MissingMandatoryValue

def fail_on_missing(cfg: Any) -> None:
    if isinstance(cfg, ListConfig):
        for x in cfg:
            fail_on_missing(x)
    elif isinstance(cfg, DictConfig):
        for _, v in cfg.items():
            fail_on_missing(v)