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

Remove restrictions on inheriting partial'd configs #290

Merged
merged 17 commits into from Aug 17, 2022

Conversation

rsokl
Copy link
Contributor

@rsokl rsokl commented Jun 23, 2022

There are restrictions on inheritance patterns permitted by builds. Specifically, one could not inherit from a PartialBuilds-type config without explicitly "opting-in" and specifying zen_partial=True on the child. And even in this latter case, one could not include zen-processing features (such as zen_meta) in the child config.

See #289 for a detailed discussion of these restrictions.

This PR lifts all of these restrictions!

Before:

The default value for zen_partial was False. There was no way for a user to inherit from a partial-builds config and produce a builds-config

parent_with_partial = builds(int, zen_partial=True)

builds(int)  # Type[Builds[Type[int]]]
builds(int, builds_bases=(parent_with_partial,)) # Not allowed
builds(int, zen_partial=False, builds_bases=(parent_with_partial,)) # Not allowed
builds(int, zen_partial=True, builds_bases=(parent_with_partial,)) # Type[PartialBuilds[Type[int]]]
builds(int, zen_partial=True, builds_bases=(parent_with_partial ,), zen_meta=dict(a=1))  # Not allowed (child uses zen-processing features)

After:

The default value for zen_partial is now None. Specifying zen_partial explicitly will determine whether the resulting config is a partial-builds, otherwise it is inherited or defaults to False

parent_with_partial = builds(int, zen_partial=True)

builds(int)  # Type[Builds[Type[int]]]
builds(int, builds_bases=(parent_with_partial,)) # Type[PartialBuilds[Type[int]]]
builds(int, zen_partial=False, builds_bases=(parent_with_partial,))  # Type[Builds[Type[int]]]
builds(int, zen_partial=True, builds_bases=(parent_with_partial,)) # Type[PartialBuilds[Type[int]]]
builds(int, zen_partial=True, builds_bases=(parent_with_partial,), zen_meta=dict(a=1))  # Type[PartialBuilds[Type[int]]]

It achieves this by changing the default value of builds(..., zen_partial) to be None instead of False.

  • None indicates that the config will not entail partial instantiation unless that behavior is inherited
  • False indicates will not entail partial instantiation under any circumstances

make_config will now also raise when the following patterns are detected in a config:

  • _zen_target is specified but _target_ does not point to hydra_zen.funcs.zen_processing. This indicates a multiple inheritance pattern that "clobbers" the zen-processing configuration pattern
  • _partial_=True is specified in conjunction with zen-processing patterns

To-Do:

  • Add error handling in make_config when inheriting _partial_=True and _zen_partial=True fields simultaneously
  • Provide public function for checking is_partial and/or validate_partial so that users can inspect whether or not a config is a partial config without needing insights into the two different protocols for partial configs.

Closes #289
Closes #288

@rsokl rsokl added the enhancement New feature or request label Jun 23, 2022
@rsokl rsokl added this to the hydra-zen 0.8.0 milestone Jun 23, 2022
@rsokl rsokl changed the title Relax partial restrictions Remove restrictions on inheriting partial'd configs Jun 23, 2022
@rsokl
Copy link
Contributor Author

rsokl commented Jul 15, 2022

@jgbos would you mind reviewing this / working off this branch during your day-to-day (if you aren't already) to confirm that you don't hit bugs? I still have some lingering to-dos, but they are relatively minor compared to the rest of this PR

@jgbos
Copy link
Contributor

jgbos commented Jul 23, 2022

@rsokl fyi, I haven't been running into any issues with the branch all week (I haven't done a lot though).

@rsokl
Copy link
Contributor Author

rsokl commented Jul 24, 2022

@jgbos great! I plan to try to tackle the outstanding To-Dos when possible and then merge this

@rsokl rsokl marked this pull request as ready for review August 17, 2022 19:41
@rsokl rsokl merged commit 749fcfc into main Aug 17, 2022
@rsokl rsokl deleted the relax-partial-restrictions branch August 17, 2022 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants