Allow components to have __init__ functions #327
Conversation
|
Unit tests were failing because my context manager did not correctly handle exceptions, and some of the tests expect one (using |
|
Looking at this code again, I think it would actually be cleaner to make the |
There was a problem hiding this comment.
Take my approval with a pinch of salt because I have only just kinda figured out how zookeeper works by looking at the code in this PR. But changes look good to me, assuming you decide to ignore my comment about using __init__ as a post_configure method being strange because I understand changing that would be a pain.
I agree. If it's easy to do. |
Currently, zookeeper will throw an error if a component defines an
__init__. This is annoying, because it makes it impossible to use zookeeper components that subclass existing classes that have one, e.g.tf.keras.models.Model.This PR changes that by:
__init__function, which will be called directly after configuration. This makes its behavior identical to__post_configure__, which is therefore deprecated. Neither function supports any arguments.__post_configure__but is not configured #280. The reason I've implemented this is that it would be very easy for bugs to appear if the user expects their__init__to have run, but the component was never configured. This means that users are now effectively forced to configure any and all zookeeper components they use, which may be overly strict in case allFields already have default values. Before making this an official release, we could therefore consider automatically callingconfigureon a component if all its fields have default values. This is tricky, however, because the user may then still configure it from the command line, and we wouldn't want the init functions to be called multiple times.