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

New feature: enable builds to bind "meta" fields #102

Closed
wants to merge 2 commits into from
Closed

Conversation

rsokl
Copy link
Contributor

@rsokl rsokl commented Sep 16, 2021

To-Do:

  • Finalize names for hydra_meta, _excluded_, and _true_target_
  • Refactor hydra_partia=True scenario to leverage the same mechanism as hydra_meta
    • Includes update to PartialBuilds protocol
  • Add support to get_target and hydrated_dataclass

The Pitch

Heh... so this is a little bit of a weird one, but I think it might be pretty useful.

This enables us to bind so-called "meta" fields to a dataclass via builds. By "meta field" I mean a field in the resulting dataclass that will not be used during instantiation.

For example, let's build a dictionary that has two typical fields, a and b, and a "meta" field c which will be excluded from the actual instantiation process.

>>> Conf = builds(dict, a=1, b=2, hydra_meta=dict(c=3))
>>> Conf.a, Conf.b, Conf.c 
(1, 2, 3)
>>> instantiate(Conf)  # notice that c is **not** included in the instantiation
{'a': 1, 'b': 2}

To peek under the hood here:

>>> print(to_yaml(Conf))
_target_: hydra_zen.funcs.pass_it   <-- (note: I plan to change this function's name)
_true_target_: builtins.dict
_excluded_:
- c
a: 1
b: 2
c: 3

Why is this useful? The meta field can be used to store a (configurable) default value, and other fields can reference it via relative interpolation.

>>> Conf = builds(dict, a="${._s1}", b="${._s2}", c="${._s2}", d="${._s1}", hydra_meta=dict(_s1=10, _s2="hi"))
>>> instantiate(Conf )
{'a': 10, 'b': 'hi', 'c': 'hi', 'd': 10}
>>> hydra_run(Conf , instantiate, overrides=["_s1=-3"]).return_value
{'a': -3, 'b': 'hi', 'c': 'hi', 'd': -3}
>>> hydra_run(builds(dict, nested=Conf ), instantiate, overrides=["nested._s1=-3"]).return_value
{'nested': {'a': -3, 'b': 'hi', 'c': 'hi', 'd': -3}}

This means that we can use the above conf at any level of our config, and the interpolated values will always hold relative to its location.

And..uhh...why is this useful? Consider this config of an image augmentation chain:

Cifar10TransformConf = builds(
    transforms.Compose,
    transforms=[
        builds(transforms.RandomCrop, size="${...size}", padding=4),
        builds(transforms.ToTensor),
        builds(
            transforms.Normalize,
            mean="${...mean}",
            std="${...std}",
        ),
    ],
    hydra_meta=dict(
        mean=[0.4914, 0.4822, 0.4465],
        std=[0.2023, 0.1994, 0.2010],
        size=32,
    ),
)

Normally size, mean, and std would either need to be hard-coded into the builds of RandomCrop (etc.) and thus be unconfigurable due to the presence of the list. Or they would need to point to values at the top-level of our experiment's config via absolute interpolation, which messes up composability and makes our configuration messier.

But now this config is truly portable! You can plug it in at any level of your config, and its default values will resolve via relative interpolation, plus you can configure these values in a natural way:

@dataclass
class ExpConfig:
    # simulating a nested config
    data: Any = builds(dict, transform=Cifar10TransformConf )
>>> hydra_run(
...    ExpConfig,
...    instantiate,
...    overrides=["data.transform.size=100", "data.transform.mean=[1,-1,0]"],
... ).return_value
{'data': {'transform': Compose(
    RandomCrop(size=(100, 100), padding=4)
    ToTensor()
    Normalize(mean=[1, -1, 0], std=[0.2023, 0.1994, 0.201])
)}}

This also means that these configs are composable via inheritance (this doesn't work on this branch yet, but it will)

MNISTTransformConf = builds(
    transforms.Compose,
    builds_bases=(Cifar10TransformConf,)
    hydra_meta=dict(
        mean=[0],
        std=[1],
        size=32,
    ),
)

@jgbos let me know what you think of this. I am not sure I am doing a good job selling/explaining this, and maybe that is a bad sign that this feature is too complicated.

It definitely would clean up some configs I have in my research. I wonder if this strikes you as a valuable feature for you as well.

If you do think this is worth having, then I will definitely want to brainstorm with you about the right names for everything. Like is hydra_meta really what we want to go with, etc.

Feel free to pull this branch and play with it to get a feel for the examples I wrote out above.

Some More Thoughts

It actually occurred to me that I could have taken the image augmentation example even further, in terms of exposing configurable interfaces:

Cifar10TransformConf = builds(
    transforms.Compose,
    transforms=["${..crop}", builds(transforms.ToTensor), "${..norm}"],
    hydra_meta=dict(
        crop=builds(transforms.RandomCrop, size=32, padding=4),
        norm=builds(
            transforms.Normalize,
            mean=(0.4914, 0.4822, 0.4465),
            std=(0.2023, 0.1994, 0.2010),
        ),
    ),
)

Now all configurable components are exposed in a nice, modular (and named) way.

>>> hydra_run(
...     Cifar10TransformConf,
...     instantiate,
...     overrides=["crop.padding=1", "crop.size=50", "norm.mean=[1, -1, 0]"],
... ).return_value
Compose(
    RandomCrop(size=(50, 50), padding=1)
    ToTensor()
    Normalize(mean=[1, -1, 0], std=[0.2023, 0.1994, 0.201])
)

Although it doesn't seem like inheritance would be quite as useful in this case... so there are pros and cons.

Attaching General Metadata

I am not sure if this would be actually useful, but I also realize that this can let people attach metadata to their configs, like maybe a version number?

builds(dict, a=2, hydra_meta=dict(version="2.0"))

Its yaml would be

_target_: hydra_zen.funcs.pass_it
_true_target_: builtins.dict
_excluded_:
- version
a: 2
version: '2.0'

@rsokl rsokl requested a review from jgbos September 16, 2021 07:28
@rsokl
Copy link
Contributor Author

rsokl commented Sep 17, 2021

@jgbos from some of our chatting, it seemed like you are in favor of this feature. Is that right? Do you have any thoughts / concerns?

If we do move forward with this, I would like to figure out good names for everything..

@Jasha10
Copy link
Contributor

Jasha10 commented Sep 17, 2021

This use of an _excluded_ flag seems related to (though not exactly the same as) some feature requests that have come up in Hydra with users asking for the ability to use the Hydra defaults list to delete nodes from Hydra's output config (e.g. facebookresearch/hydra#1827).
In Hydra we have not enabled this because (at this point) the implementation would be challenging.

@rsokl
Copy link
Contributor Author

rsokl commented Sep 18, 2021

@Jasha10 thanks for pointing me to that issue. I would be glad to get your input on / impression of this feature; it would be great if it could help address common issues for Hydra users.

Regarding facebookresearch/hydra#1827, it seems like the issue the OP is having really comes down to the cumbersome nature of having to construct groups/yamls by hand; they say:

Having a config group for any node that has a _target_ is a nightmare.

I can see why they would want to be able to delete nodes in their case. That being said, I have to wonder if simply usingbuilds and ConfigStore might make it feasible for them to simply create groups as-needed, which would probably be the most natural solution for them.

@jgbos
Copy link
Contributor

jgbos commented Sep 18, 2021

@rsokl yes I am in favor of this based on our conversations!

@jgbos jgbos closed this Sep 18, 2021
@jgbos jgbos reopened this Sep 18, 2021
@Jasha10
Copy link
Contributor

Jasha10 commented Sep 18, 2021

... your input on / impression of this feature ...

Your pitch above is compelling!
A few thoughts:

This pass_it function is introducing a layer of indirection -- the removal of the _excluded_ args is a postprocessing step that occurs after *args and **kwargs are instantiated, but before _true_target_ gets called.

The fact that this post-processing is encoded in the config (via the _target_ pointing to pass_it) means that the config is now coupled to hydra-zen.
For example:

cfg_meta = builds(dict, hydra_meta={"X": 123})  # _target_ points into the hydra-zen library
cfg_nometa = builds(dict, x=123)                # _target_ points to builtins.dict

In theory, one could dump cfg_nometa to yaml and email it to a friend who uses hydra but who does not know about hydra-zen. Meanwhile, cfg_meta will not work for the friend unless they install hydra-zen. (The same goes for hydra_partial=True). I assume this is not a problem for your working group :)

Using this layer of indirection, one gains the power to perform arbitrary per-node post-processing before the _true_target_ is called. I wonder what other post-processing steps (besides nixing of _excluded_ args) might be useful...

Regarding facebookresearch/hydra#1827, it seems like the issue the OP is having really comes down to the cumbersome nature of having to construct groups/yamls by hand ...

I think you're right that automating the construction of groups would ease the pain-point.
In addition, I feel that the OP's feature request showed a desire to achieve some degree of orthogonality between the _target_ that is selected and the args that are passed to that target.
From the OP's example:

  • use experiment1.yaml to switch what norm is used as the _target_ (GroupNorm vs BatchNorm2d)
  • use my_awesome_model.yaml to define common arguments that could be passed to whatever target is used (num_groups=32, num_features=64)
  • ensure that the combination of experiment1.yaml and my_awesome_model.yaml does not cause an error: drop whatever arguments cannot be handled by the selected norm (e.g. BatchNorm2d does not take a num_groups parameter).

In theory, this is also something that could be achieved automatically via an indirection layer similar to pass_it; the indirection layer would drop whatever args do not appear in the function signature of _true_target_. Meanwhile, the OP's approach was to hard-code on a per-target basis the list of args were to be dropped.

@hal-314

@@ -607,6 +611,24 @@ def builds(
_utils.field(default=just(target), init=False),
),
]
elif hydra_meta:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to handle the case where both hydra_partial is True and hydra_meta is not None?

Copy link
Contributor Author

@rsokl rsokl Sep 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! I definitely do need to handle this case. This is just a crude WIP right now.

I plan to refactor how we handle hydra_partial=True so that it also looks for _true_target_ instead of _partial_target_. I want to converge on a common interface for any/all post-processing functions that hydra-zen ships.

@rsokl
Copy link
Contributor Author

rsokl commented Sep 18, 2021

means that the config is now coupled to hydra-zen

Yep, this is something that we are okay (happy) with, and hopefully others would be as well. Since hydra-zen is pip-installable and only introduces a typing_extensions dependency, we like to think that this coupling will be tolerable to most folks.

To me, this seems preferable over being coupled to custom resolvers; not only do those functions need to be made available to users (making the email scenario you laid out much stickier), but the user also has to take care to register them with omegaconf before instantiating the config!

This all being said, I realize that we should document that using certain features (hydra_partial=True) will introduce this dependency. Thanks for bringing this up 😄

In theory, this is also something that could be achieved automatically via an indirection layer similar to pass_it; the indirection layer would drop whatever args do not appear in the function signature of true_target

I was starting to think about this.. It would definitely work, but I am not sure that I would like supporting this. It seems to endorse generally-bad practices. Say that I am configuring a class that has a default kwarg, num_neurons=10, and I that I specify builds(..., num_nuerons=100) (note the typo). If I were using this "adaptive drop" method, the misnamed field would silently be ignored, and I would unwittingly be using num_neurons=10 in my run.

I realize you probably also are wary of this, hence your "in theory" premise, but I just wanted to lay out my thoughts/reservations while they were still fresh.

Using this layer of indirection, one gains the power to perform arbitrary per-node post-processing before the _true_target_ is called. I wonder what other post-processing steps (besides nixing of _excluded_ args) might be useful...

Indeed, this is really convenient and quite powerful. And this method can also be used for pre-processing (e.g. more-robust type validation) as well.

Some thoughts that I have had:

  • Let users pass in a list of their own pre/post-processing functions that each are configurable and that get instantiated prior to operating on the target.
  • Enable more robust runtime type validation. Something like:
      1. Check target's signature and config-values (which will already have been recursively-instantiated) for bad types.
      1. Instantiate target with validated inputs
    • I have imagined leveraging something like beartype or pydantic to handle all of this high-fidelity type checking. This would enable the whole rich ecosystem of types and validation to be leveraged in conjunction with using Hydra. It would also be encouraging to users who have already spent time adding incisive annotations to their interfaces. This isn't so simple though, since omegaconf will currently flag all of those richer types.

Finally (sorry for the long post), if there are any particular names for fields (e.g. _partial_target_ or _excluded_) that hydra_zen can adopt to synergize with (or avoid conflict with) future capabilities of Hyda, we would be eager to do so. For example, it is my understanding that something like _partial_target_ might be supported by Hydra in the future.

@rsokl
Copy link
Contributor Author

rsokl commented Sep 18, 2021

@jgbos I added a to-do list to the top of the PR. The main thing I would like input on is for naming things.

@rsokl rsokl mentioned this pull request Sep 18, 2021
@Jasha10
Copy link
Contributor

Jasha10 commented Sep 19, 2021

To me, this seems preferable over being coupled to custom resolvers; not only do those functions need to be made available to users (making the email scenario you laid out much stickier), but the user also has to take care to register them with omegaconf before instantiating the config!

Agreed!
While custom resolvers are also a processing step that the user can encode in their config, I feel that they have a different intended use-case: they let yaml power-users create a dsl for transforming their config data inline, directly in the yaml file. The resolvers can be chained/composed (e.g. "node2: ${fn1:${fn2:${node1}}}"), and they can operate on the config tree via the resolver's _parent_ and _root_ arguments.
Meanwhile, if the config is being composed in python (rather than in yaml), hydra-zen is much much cleaner.

... This isn't so simple though, since omegaconf will currently flag all of those richer types.

Yup. I think there are some fundamental aspects of OmegaConf's design that would make it very difficult to support arbitrary rich types while also maintaining backward compatibility.

Finally (sorry for the long post), if there are any particular names for fields (e.g. _partial_target_ or _excluded_) that hydra_zen can adopt to synergize with (or avoid conflict with) future capabilities of Hyda, we would be eager to do so. For example, it is my understanding that something like _partial_target_ might be supported by Hydra in the future.

Yes, there seems to be active discussion around partial capabilities in facebookresearch/hydra#1283.
Here is my compendium of current Hydra keywords (with links to the parts of the docs where they appear):

The first 6 of these only have import when used as part of a string in a defaults list, and the last 4 only matter when used with instantiate. I think there may have been some discussion about introducing a _kwargs_ keyword for instantiate too.

The most robust way I can think of to avoid future conflicts would be to avoid the leading/trailing underscore pattern.

@rsokl
Copy link
Contributor Author

rsokl commented Sep 19, 2021

The most robust way I can think of to avoid future conflicts would be to avoid the leading/trailing underscore pattern.

This is a good point. I plan to change all hydra-zen-specific fields to be patterned as _zen_XX. This would obviously avoid collisions with future Hydra features, and would help make obvious to users which fields are related to specialized behavior from hydra-zen vs Hydra itself. I should have thought of this earlier (which is an ever-echoing sentiment whenever a new project is taking shape).

Fortunately, I can change the implementation of builds(..., hydra_partial=True) without breaking backwards compatibility of yamls that were produced with _partial_target_

Here is my compendium of current Hydra keywords

This is very useful. Thank you for this! 😄

While custom resolvers are also a processing step that the user can encode in their config, I feel that they have a different intended use-case: they let yaml power-users create a dsl for transforming their config data inline

This is well-put. I should provide a description along these lines in hydra-zen's docs to try to distinguish these use-cases.

I wonder what other post-processing steps (besides nixing of excluded args) might be useful...

Definitely let us know if you think of any ideas!

@rsokl
Copy link
Contributor Author

rsokl commented Sep 24, 2021

Closing this to break it into to parts:

1: Migrating to the _zen_xx naming scheme for fields that we occupy

  • Per @Jasha10 's recommendation, we want to avoid potential future-conflicts with Hydra keeping away from the _xx_ naming scheme.
  • Thus specifying hydra_partial=True will no longer occupy _partial_target_. Fortunately, we can make this change while maintaining backwards-compatibility with any yamls that use this old paradigm.
  • We will use this opportunity to adopt a common interface that we will use to facilitate our additional processing. E.g. it will be the machinery behind hydra_partial, hydra_meta, their combination, and future features that hydra-zen provides. Trying to implement these with separate functions, but them making them composable, would be a nightmare.

2: Adding the hydra_meta feature

@jgbos and I agreed that hydra_meta is a reasonable name. Also considered was

  • hydra_excluded. We decided against it because this feels like it should be used as builds(dict, a=1, b=2, hydra_excluded=("b",)) instead of builds(dict, a=1, hydra_excluded=dict(b=2)).
  • hydra_local. This does get at the fact that we are enabling the user to create a "local namespace" that they can then exploit... but this is too subtle for our liking.

hydra_meta has the benefit not being eye-catching to new users, who probably don't need it anyway, and yet sensible to power-users.

@rsokl rsokl closed this Sep 24, 2021
@rsokl rsokl mentioned this pull request Sep 30, 2021
@rsokl
Copy link
Contributor Author

rsokl commented Oct 2, 2021

Yup. I think there are some fundamental aspects of OmegaConf's design that would make it very difficult to support arbitrary rich types while also maintaining backward compatibility.

@Jasha10 I got this working! Check out #117

@rsokl rsokl deleted the vestigial-fields branch January 9, 2022 16:38
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 this pull request may close these issues.

None yet

3 participants