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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Parsing list of configurations with a structured config #1389

Closed
2 tasks done
jzazo opened this issue Feb 9, 2021 · 11 comments
Closed
2 tasks done

[Bug] Parsing list of configurations with a structured config #1389

jzazo opened this issue Feb 9, 2021 · 11 comments
Labels
bug Something isn't working invalid This doesn't seem right list composition

Comments

@jzazo
Copy link

jzazo commented Feb 9, 2021

馃悰 Bug

Description

I cannot parse a list of configs as attribute in a schema that requires a list type. Using latest dev version of Hydra 1.1. I think this feature is supported, although I have not found examples in the documentation.

Checklist

  • I checked on the latest version of Hydra
  • I created a minimal repro.

To reproduce

** Minimal Code/Config snippet to reproduce **
This snipped is almost exactly as the documented example. Changes are in line 36 of my_app.py where I specify a List type, and config.yaml, where I input a list instead of an entry:

from dataclasses import dataclass, field
from typing import List

import hydra
from hydra.core.config_store import ConfigStore
from omegaconf import MISSING, OmegaConf


@dataclass
class DBConfig:
    driver: str = MISSING
    host: str = "localhost"
    port: int = MISSING


@dataclass
class MySQLConfig(DBConfig):
    driver: str = "mysql"
    port: int = 3306
    user: str = MISSING
    password: str = MISSING


@dataclass
class PostGreSQLConfig(DBConfig):
    driver: str = "postgresql"
    user: str = MISSING
    port: int = 5432
    password: str = MISSING
    timeout: int = 10


@dataclass
class Config:
    db: List[DBConfig] = field(default_factory=list)
    debug: bool = False


cs = ConfigStore.instance()
cs.store(name="base_config", node=Config)
cs.store(group="db", name="base_mysql", node=MySQLConfig)
cs.store(group="db", name="base_postgresql", node=PostGreSQLConfig)


@hydra.main(config_path="conf", config_name="config")
def my_app(cfg: Config) -> None:
    print(OmegaConf.to_yaml(cfg))


if __name__ == "__main__":
    my_app()

and config.yaml specifies a list input, instead of a single entry:

defaults:
  - base_config
  - db: 
    - mysql
    - postgresql

debug: true

** Stack trace/error message **

$ python src/hydra_listconfigs/script.py

Cannot merge DictConfig with ListConfig
    full_key: 
    object_type=Config

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

Expected Behavior

I expected that db group would be a ListConfig of two DBConfig elements and the whole schema would work, but it fails.

System information

  • Hydra Version : 1.1.0.dev3
  • Python version : Python 3.6.8
  • Operating system : Ubuntu 18.04

Additional context

I first asked this question in stackoverflow.

@jzazo jzazo added the bug Something isn't working label Feb 9, 2021
@omry
Copy link
Collaborator

omry commented Feb 9, 2021

I expected that db group would be a ListConfig of two DBConfig elements and the whole schema would work, but it fails.

This is not how it works. List composition is not supported.

You can create a dict with both DBConfigs in 1.1 per the link you received your stack overflow question.

@omry omry added the invalid This doesn't seem right label Feb 9, 2021
@jzazo
Copy link
Author

jzazo commented Feb 9, 2021

Do you think list composition could be supported for Hydra 1.1., or in the future?

I ask because some models accept list of objects as attributes. This feature would allow automatic list creation (instead of a dictionary), and recursive instantiation of the model + lists, so it may be a common use case.

I was thinking of parsing the list even as a dictionary, and indexing the first element as db.0, db.1, etc. Do you think there is a way to get this parsing behavior easily? Thanks.

@omry
Copy link
Collaborator

omry commented Feb 9, 2021

Do you think list composition could be supported for Hydra 1.1., or in the future?
Probably not in 1.1, maybe in the future.

One thing that you can do now is to use interpolation to simulate it:

dict:
  item1:
     a: 10
  item2:
    b: 20

as_list:
  - ${dict.item1}
  - ${dict.item2}

OmegaConf 2.1 will support more powerful custom resolvers. It's possible that they will enable generating as_list automatically from dict. (cc @odelalleau).

I was thinking of parsing the list even as a dictionary, and indexing the first element as db.0, db.1, etc. Do you think there is a way to get this parsing behavior easily? Thanks.

I don't understand what your question.

@jzazo
Copy link
Author

jzazo commented Feb 9, 2021

Ok, thanks for your suggestions.

I don't understand what your question.

It was not a clear question, maybe I framed in between suggestion/question. Somewhere in an issue I read that to override a default value in a list, you could access as db.0 for the first element, db.1 for the second, etc. My suggestion was to treat a composition list as a composition dictionary with integer keys. Hope this clarifies the idea a bit, or maybe it does not make sense, or omegaconf 2.1 resolvers would take care of it more elegantly.

To conclude, an example of a similar use case I have, is the hydra-optuna-sweeper. The search_space key is annotated as a dictionary, but its elements, which are of DistributionConfig type, are not (yet) instantiated recursively, rather manually in the OptunaSweeper. The DistributionConfigs are not validated against a config schema in this case, maybe because they are a variable number. My use case is similar, with varying number of elements (a list rather than a dictionary). For now, I can specify a hierarchy of configs and instantiating them manually, as OptunaSweeper does with different variables. If that code is updated, or the recursive documentation reflects similar case and validation with schemas, then I can replicate.

Thanks for your support. I will close the issue now.

@jzazo jzazo closed this as completed Feb 9, 2021
@odelalleau
Copy link
Collaborator

odelalleau commented Feb 9, 2021

OmegaConf 2.1 will support more powerful custom resolvers. It's possible that they will enable generating as_list automatically from dict. (cc @odelalleau).

Not in my current implementation because resolvers systematically wrap their output in a ValueNode. However I suspect that if we used _node_wrap() instead then it should be possible. Not going to change it now though -- I'd rather keep it for later.

Edit: read my other comment below though

@omry
Copy link
Collaborator

omry commented Feb 9, 2021

It was not a clear question, maybe I framed in between suggestion/question. Somewhere in an issue I read that to override a default value in a list, you could access as db.0 for the first element, db.1 for the second, etc. My suggestion was to treat a composition list as a composition dictionary with integer keys. Hope this clarifies the idea a bit, or maybe it does not make sense, or omegaconf 2.1 resolvers would take care of it more elegantly.

I see what you mean.
I think it does not make for a good api for composition.
imagine you want to select 2 out of 3 items.

1: 
  a: 10
2: 
  a: 20
3: 
  a: 30

Depending on which one you chose, you may end up with a hole in your list.
It also requires pretty difficult coordination. It's hard to know what should be the key for a new item (you have to go over all files and see which one has the largest index).

@odelalleau
Copy link
Collaborator

OmegaConf 2.1 will support more powerful custom resolvers. It's possible that they will enable generating as_list automatically from dict. (cc @odelalleau).

Not in my current implementation because resolvers systematically wrap their output in a ValueNode. However I suspect that if we used _node_wrap() instead then it should be possible. Not going to change it now though -- I'd rather keep it for later.

I think this should be possible now, with oc.dict.keys, see https://omegaconf.readthedocs.io/en/2.1_branch/custom_resolvers.html#oc-dict-keys-value

@masteranza
Copy link

masteranza commented Aug 17, 2021

@odelalleau thanks! Actually, @omry found quite elegant solution for me yesterday on zulipchat: https://hydra-framework.zulipchat.com/#narrow/stream/213879-Hydra/topic/.E2.9C.94.20Typechecking.20.26.20Structured.20Configs

@cjsg
Copy link

cjsg commented Jul 18, 2022

Could someone explain why the code from @jzazo doesn't or even shouldn't work? Isn't it essentially the same than what is documented in the multiple-config-groups example from the docs, with StructuredConfigs instead of config files?

Actually, I have tried to re-implement the multiple-config-groups example from the docs with structured configs (see below), and I get the same error than @jzazo. But why? And is there a way to specify multiple (i.e., a list of) configs with Structured Configs?

from dataclasses import dataclass, field
from typing import Any, Dict, List

import hydra
from hydra.core.config_store import ConfigStore
from omegaconf import OmegaConf


@dataclass
class ApacheConfig:
    site: List[Any] = field(default_factory=list)
    host: str = "localhost"
    port: int = 443


@dataclass
class FbConfig:
    fb: Dict[str, str] = field(default_factory=lambda: {"domain": "fb.com"})


@dataclass
class GoogleConfig:
    google: Dict[str, str] = field(default_factory=lambda: {"domain": "google.com"})


cs = ConfigStore.instance()
cs.store(name="apache", group="server", node=ApacheConfig)
cs.store(name="fb", group="server/site", node=FbConfig)
cs.store(name="google", group="server/site", node=GoogleConfig)


@hydra.main(version_base=None, config_path="conf", config_name="config")
def my_app(cfg):
    print(OmegaConf.to_yaml(cfg))


if __name__ == "__main__":
    my_app()

with the following config.yaml file:

defaults:
  - server: apache
  - server/site:
    - fb
    - google

@jzazo
Copy link
Author

jzazo commented Aug 22, 2022

I don't know why it does not work, but the feature request is being tracked, and hopefully it will be released in Hydra 1.3.0.

@Jasha10
Copy link
Collaborator

Jasha10 commented Aug 25, 2022

Could someone explain why the code from @jzazo doesn't or even shouldn't work? Isn't it essentially the same than what is documented in the multiple-config-groups example from the docs, with StructuredConfigs instead of config files?

Yes, the syntax is the same as in multiple-config-groups example.
The error message "Cannot merge DictConfig with ListConfig" from the original post is expected. The error occurs because:

  • the defaults list from config.yaml, i.e. defaults: [..., {db: [mysql, postgresql]}] creates a config by merging together the PostGreSQLConfig and the PostGreSQLConfig configs; the result is a DictConfig instance (rather than a ListConfig instance).
  • Since the result of merging PostGreSQLConfig and PostGreSQLConfig is a DictConfig, the value cannot be assigned to the db field of the Config dataclass. The type annotation for the db field of the Config dataclass specifies that the value must be a list (not a dict).

And is there a way to specify multiple (i.e., a list of) configs with Structured Configs?

See here and here for my recommended workaround based on Omry's comments above. You can use a defaults field in your structured config to implement the prescription from that workaround.

the feature request is being tracked, and hopefully it will be released in Hydra 1.3.0.

Yes, this is certainly on our radar. Most likely we will not get to this for Hydra 1.3, however.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working invalid This doesn't seem right list composition
Projects
None yet
Development

No branches or pull requests

6 participants