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

Feature idea: metadata option to omit field #984

Closed
bottler opened this issue Jul 28, 2022 · 5 comments
Closed

Feature idea: metadata option to omit field #984

bottler opened this issue Jul 28, 2022 · 5 comments

Comments

@bottler
Copy link

bottler commented Jul 28, 2022

I make extensive use of structured config with dataclasses, where the classes have a lot of their own logic. Their fields are generally their configuration, but I'd like to have some fields which are linked to program logic but not to configuration, and some of these I want to allow passing to __init__.

Proposed solution

My current idea is to change OmegaConf.structured so that it looks for some indication in a field's metadata that it should ignore that field. For example, we could decide to check for a value "omegaconf_ignore". Then the following would print {"a": 3, "b": 3}.

from dataclasses import dataclass, field

@dataclass
class A:
    a: int = 3
    b: int = field(default=3, metadata = {"omegaconf_ignore": False})
    c: int = field(default=3, metadata = {"omegaconf_ignore": True})

print(OmegaConf.structured(A))  # prints {"a": 3, "b": 3}

(If an ignored field was undefaulted, then you would not be able to call to_object() on the config.)

Would this be a good idea? Could I make a PR?

@Jasha10
Copy link
Collaborator

Jasha10 commented Jul 28, 2022

Hi @bottler,
I'd like to get a better feel for how you are using structured configs. How do you translate between DictConfig instances and dataclass instances? Are you using OmegaConf.to_object?
Or do you call the constructor for A directly in your code (e.g. A(**cfg))?

@bottler
Copy link
Author

bottler commented Jul 28, 2022

I generally call the constructor directly.

@bottler
Copy link
Author

bottler commented Jul 28, 2022

The reason we avoid to_object is because of considerations about what should happen when a notebook user redefines a class.

@Jasha10
Copy link
Collaborator

Jasha10 commented Jul 28, 2022

Adding a check for metadata["omegaconf_ignore"] seems reasonable to me. I'll bring this up with the rest of the OmegaConf dev team next week to see if there is consensus.

One detail to iron out: how would "omegaconf_ignore" interact with OmegaConf.to_object? Here is an example using the dataclass A from your original post:

cfg = OmegaConf.structured(A)
assert cfg == {'a': 3, 'b': 3}  # information regarding 'c' is discarded
OmegaConf.to_object(cfg)  # What happens here? What value for 'c' is passed to `A.__init__`?

My instinct is that, in my example above, OmegaConf.to_object(cfg) should succeed if the dataclass A has a default value for field c. If c does not have a default value, then then invoking A(a=3, b=3) will cause a TypeError which we can pass through to the user.

Details for potential implementation

If you make a PR, here is some information that will be useful:

  • The behavior change should be the same for dataclasses and for attr classes
  • As for where in the OmegaConf codebase to check for "omegaconf_ignore" metadata, here are the routines that would need modification:
    • omegaconf._utils.get_dataclass_init_field_names
    • omegaconf._utils.get_dataclass_data
    • omegaconf._utils.get_attr_class_init_field_names
    • omegaconf._utils.get_attr_data
  • A PR would ideally mention the "omegaconf_ignore" API somewhere in docs/source/structured_config.rst
  • I think that testing for this feature should include at minimum:
    • adding an example dataclass to tests/structured_conf/data/dataclasses.py, and adding an equivalent attr class example to tests/structured_conf/data/attr_classes.py. The example class A from your original post (renamed to something more descriptive) should be sufficient, though a field d: int = field(metadata={"omegaconf_ignore": True}) with no default value should be added. These example classes would be tested in tests/structured_conf/test_structured_config.py to confirm the following:
      • OmegaConf.structured(A) == {'a': 3, 'b': 3}
      • OmegaConf.structured(A(a=0, b=1, c=2, d=3)) == {'a': 0, 'b': 1}
    • There should be an integration test in tests/test_to_container.py confirming that interaction with OmegaConf.to_object is as expected.

@Jasha10
Copy link
Collaborator

Jasha10 commented Aug 18, 2022

Closed by PR #988.

@Jasha10 Jasha10 closed this as completed Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants