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

clean up imports and add missing funcs #1

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

ANaka
Copy link

@ANaka ANaka commented Jan 3, 2023

Purpose

This PR is meant to stay within the cellular-longevity fork of hydra-zen. The purpose is to get the add_conf and ConfMode functionality working with the latest version of hydra zen

To-do

  • Read previous implementation of like and articulate how we differ from that if we do. See also here
  • Find a workaround for the deprecation of builds_bases. Most important item is to recapitulate the functionality of the __call__ method in our ZenExtras base class
    • Our ZenExtras was pretty loaded with features; may make sense to strip that down or create a simpler version
  • Examine the new zen store functionality and determine whether to replace the .store methods that we put into our ZenExtras
  • Build out test suite

@shababo
Copy link

shababo commented Jan 13, 2023

  • Find a workaround for the deprecation of builds_bases. Most important item is to recapitulate the functionality of the __call__ method in our ZenExtras base class

Looks like we didn't even need to use make_custom_builds_fn and can just use builds directly, e.g.:
conf = builds(wrapped_cls, populate_full_signature=True, builds_bases=(ZenExtras,), zen_meta=zen_meta_defaults, **kwargs)



@dataclass
class BatteriesIncludedConf:
class ZenExtras:
Copy link
Author

Choose a reason for hiding this comment

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

we should talk about this, it looks like some of the changes here are undoing some purposeful refactoring I put in place

@rsokl
Copy link

rsokl commented Jan 17, 2023

Hello, I just stumbled across this fork -- there are some neat ideas here! Don't hesitate to open Discussion/Issue threads on the main hydra-zen repo if you'd ever like to chat about how hydra-zen can better facilitate your work 😄 . I'm keen to understand the ways people are using the library so that we add high-impact features. (But also no worries if you just want to stay focused on your fork)

Find a workaround for the deprecation of builds_bases

Just FYI, hydra-zen 0.9.0 made it possible to effectively specify builds_bases in make_custom_builds_fn again, but via zen_dataclass

from hydra_zen import make_custom_builds_fn, builds, to_yaml

Parent = builds(dict, x=1)
cbuilds = make_custom_builds_fn(zen_dataclass={'bases': (Parent,)})

Cheers!

@ANaka
Copy link
Author

ANaka commented Jan 17, 2023

Don't hesitate to open Discussion/Issue threads on the main hydra-zen repo if you'd ever like to chat about how hydra-zen can better facilitate your work smile .

@rsokl - thanks for reaching out - we were planning on doing exactly that! We built pinned to 0.7 so our thought was to get stuff a little more up to date with the latest version first. I think we have the piece with builds_bases/zen_dataclass sorted out - thanks for the FYI. It looks to me like some of the things we were doing with ConfigStore might be redundant with what ZenStore is doing and so I'd like to look at that a little more and do any necessary reconciliation. We'll open a thread once that's handled!

@ANaka
Copy link
Author

ANaka commented Jan 19, 2023

I reverted back to some of the refactoring I did, mainly the bits that split ZenExtras into two classes. Conf/@add_conf is a simplified version of it that only retains the basic functionality (attaching a configuration dataclass to decorated classes) while BatteriesIncludedConf/@add_batteries has all the convenience methods included as well. (open to suggestion on naming)

I also updated the notebook you added - the basic interaction with ConfMode works if you use @add_batteries now. Will look at reconciling the tests later.

Looks like we didn't even need to use make_custom_builds_fn and can just use builds directly, e.g.:
conf = builds(wrapped_cls, populate_full_signature=True, builds_bases=(ZenExtras,), zen_meta=zen_meta_defaults, **kwargs)

@shababo good callout - I think that makes sense and kept this change.

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.

3 participants