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

Deprecate subclassing Dict #663

Closed
Jasha10 opened this issue Apr 6, 2021 · 6 comments · Fixed by #707
Closed

Deprecate subclassing Dict #663

Jasha10 opened this issue Apr 6, 2021 · 6 comments · Fixed by #707
Milestone

Comments

@Jasha10
Copy link
Collaborator

Jasha10 commented Apr 6, 2021

It is proposed to deprecate and then remove support for subclassing Dict in structured configs.

The motivations are:

  • we are not aware of much use of this feature
  • The feature adds maintenance burden
  • There are some cases where the API design is not obvious (e.g. discussion in OmegaConf.to_object: Instantiate structured configs #502 on handling extra fields when converting from DictConfig to dataclass/attrs class).

This is related to #657, which proposes to remove support for subclassing Dict[Any, ...] and only allow subclassing Dict[str, ...].

@omry omry added this to the OmegaConf 2.1 milestone Apr 6, 2021
Jasha10 added a commit to Jasha10/omegaconf that referenced this issue May 8, 2021
@Jasha10
Copy link
Collaborator Author

Jasha10 commented May 8, 2021

Migration Suggestions

For those who use are currently using this feature, here are a few ways to migrate away from subclassing typing.Dict in your structured configs:

Use a Dict-typed field to hold extra data

One option for migration is to add a Dict-typed field to your Structured Config class. For example, suppose we have a subclass of typing.Dict as below:

from dataclasses import dataclass
from typing import Dict
from omegaconf import OmegaConf

# The class below uses the deprecated Dict subclass feature
@dataclass
class DictSubclass(Dict[str, str]):
    x: int = 123

cfg = OmegaConf.structured(DictSubclass)
cfg.y = "abc"

assert cfg.x == 123
assert cfg.y == "abc"

This can be replaced with:

from dataclasses import field

# Here we use a Dict-typed `data` field instead of subclassing Dict
@dataclass
class ExtraField():
    x: int = 123
    data: Dict[str, str] = field(default_factory=dict)

cfg = OmegaConf.structured(ExtraField)
cfg.data.y = "abc"

assert cfg.x == 123
assert cfg.data.y == "abc"

Use a non-structured DictConfig object

If you are wiling to give up runtime type-checking, you might consider replacing your structured config with a non-structured config.

cfg = OmegaConf.create({"x": 123})
cfg.y = "abc"

assert cfg.x == 123
assert cfg.y == "abc"

Set the struct flag to False

If you are not willing to give up runtime type checking, but you still want to add fields at the top level of your Structured Config object, you can consider explicitly setting your config's 'struct' flag to False.

@dataclass
class MyConf():
    x: int = 123

cfg = OmegaConf.structured(MyConf)
OmegaConf.set_struct(cfg, False)
cfg.y = "abc"

assert cfg.x == 123
assert cfg.y == "abc"

Be aware that explicitly setting 'struct' to False has some implications for container objects or structured config objects that are nested within cfg (because of the way that nested configs inherit configuration flags from their parents).

As an alternative to permanently setting 'struct' to False as above, you can use the open_dict context manager to achieve the same effect temporarily:

from omegaconf import open_dict
cfg = OmegaConf.structured(MyConf)
with open_dict(cfg):
    cfg.y = "abc"
assert cfg.y == "abc"

@odelalleau
Copy link
Collaborator

I can't find a good workaround for this situation:

@dataclass
class Config1(Dict[str, Any]):
   x: int = 0

@dataclass
class Config2(Dict[str, Any]):
   y: int = 0

cfg = OmegaConf.merge(Config1, Config2)

I wish the following would work (currently it doesn't):

@dataclass
class Config1:
   x: int = 0

@dataclass
class Config2:
   y: int = 0

    
cfg = OmegaConf.structured(Config1)
with open_dict(cfg):
    cfg = OmegaConf.merge(cfg, Config2)

@Jasha10
Copy link
Collaborator Author

Jasha10 commented May 20, 2021

When I try to run the code above, I get this error:

Traceback (most recent call last):
    ...
omegaconf.errors.ValidationError: Merge error: Config2 is not a subclass of Config1. value: {'y': 0}
    full_key:
    object_type=Config1

Is this a bug? I feel that it makes sense to allow merging different structured configs in the context of open_dict. @omry, what do you think?

@omry
Copy link
Owner

omry commented May 20, 2021

@odelalleau, you can merge a dict like config onto a Structured Config, or another Structured Config of the same type or of a subclass.

@Jasha10, I dunno - this has nothing to do with struct mode. What are you expecting the resulting underlying type to be?

@Jasha10
Copy link
Collaborator Author

Jasha10 commented May 20, 2021

What are you expecting the resulting underlying type to be?

Good point.

@odelalleau
Copy link
Collaborator

Moving the discussion to #721 to avoid polluting this issue

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

Successfully merging a pull request may close this issue.

3 participants