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

Add __configure__ function to component #210

Merged
merged 19 commits into from
Dec 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 51 additions & 11 deletions zookeeper/core/component.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
"""
Components are generic, modular classes designed to be easily configurable.
"""Components are generic, modular classes designed to be easily configurable.

Components have configurable fields, which can contain either generic Python
objects or nested sub-components. These are declared with class-level Python
Expand Down Expand Up @@ -114,8 +113,7 @@ def _type_check_and_maybe_cache(instance, field: Field, result: Any) -> None:


def _wrap_getattribute(component_cls: Type) -> None:
"""
The logic for this overriden `__getattribute__` is as follows:
"""The logic for this overriden `__getattribute__` is as follows:

During component instantiation, any values passed to `__init__` are stored
in a dict on the instance `__component_instantiated_field_values__`. This
Expand Down Expand Up @@ -304,6 +302,51 @@ def wrapped_fn(instance) -> List[str]:
component_cls.__dir__ = wrapped_fn


def _wrap_configure(component_cls: Type) -> None:
if hasattr(component_cls, "__configure__"):
if not callable(component_cls.__configure__):
raise TypeError(
"The `__configure__` attribute of a @component class must be a "
"method."
)
call_args = inspect.signature(component_cls.__configure__).parameters
configure_args = inspect.signature(configure).parameters

if not all(
arg_param.kind in (arg_param.VAR_POSITIONAL, arg_param.VAR_KEYWORD)
or arg_name in configure_args
or arg_name == "self"
for arg_name, arg_param in call_args.items()
):
raise TypeError(
"The `__configure__` method of a @component class must match the "
f"arguments of `configure()`, but `{component_cls.__name__}.__configure__`"
f" accepts arguments {tuple(call_args)}. Valid "
f"arguments: ({', '.join(configure_args)})"
)

fn = component_cls.__configure__

@functools.wraps(fn)
def wrapped_configure(instance, *args, **kwargs):
fn(instance, *args, **kwargs)
if not instance.__component_configured__:
raise ValueError(
f"`{instance.__component_name__}` remains unconfigured after "
"calling __configure__! Make sure to call "
"`configure(self, conf, **kwargs)` at the end of this function."
)

component_cls.__configure__ = wrapped_configure

else:
component_cls.__configure__ = __component__configure__


def __component__configure__(instance, *args, **kwargs):
configure(instance, *args, **kwargs)


#################################
# Pretty string representations #
#################################
Expand Down Expand Up @@ -370,9 +413,7 @@ def __component_str__(instance):


def __component_init__(instance, **kwargs):
"""
Accepts keyword-arguments corresponding to fields defined on the component.
"""
"""Accepts keyword-arguments corresponding to fields defined on the component."""

# Use the `kwargs` to set field values.
for name, value in kwargs.items():
Expand Down Expand Up @@ -487,6 +528,7 @@ def component(cls: Type):
_wrap_setattr(cls)
_wrap_delattr(cls)
_wrap_dir(cls)
_wrap_configure(cls)

# Components should have nice `__str__` and `__repr__` methods.
cls.__str__ = __component_str__
Expand All @@ -506,8 +548,7 @@ def configure(
name: Optional[str] = None,
interactive: bool = False,
):
"""
Configure the component instance with parameters from the `conf` dict.
"""Configure the component instance with parameters from the `conf` dict.

Configuration passed through `conf` takes precedence over and will
overwrite any values already set on the instance - either class defaults
Expand Down Expand Up @@ -723,8 +764,7 @@ def configure(
for a, b in conf.items()
if a.startswith(f"{field.name}.")
}
configure(
sub_component_instance,
sub_component_instance.__configure__(
field_name_scoped_conf,
name=full_name,
interactive=interactive,
Expand Down
134 changes: 98 additions & 36 deletions zookeeper/core/component_test.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import abc
import re
from typing import List, Tuple
from unittest.mock import patch

Expand All @@ -21,9 +22,7 @@ class A:


def test_non_class_decorate_error():
"""
An error should be raised when attempting to decorate a non-class object.
"""
"""An error should be raised when attempting to decorate a non-class object."""
with pytest.raises(
TypeError, match="Only classes can be decorated with @component."
):
Expand All @@ -34,9 +33,7 @@ def fn():


def test_abstract_class_decorate_error():
"""
An error should be raised when attempting to decorate an abstract class.
"""
"""An error should be raised when attempting to decorate an abstract class."""
with pytest.raises(
TypeError, match="Abstract classes cannot be decorated with @component."
):
Expand All @@ -49,10 +46,8 @@ def foo(self):


def test_init_decorate_error():
"""
An error should be raised when attempting to decorate a class with an
`__init__` method.
"""
"""An error should be raised when attempting to decorate a class with an `__init__`
method."""
with pytest.raises(
TypeError,
match="Component classes must not define a custom `__init__` method.",
Expand All @@ -66,11 +61,9 @@ def __init__(self, a, b=5):


def test_no_init(ExampleComponentClass):
"""
If the decorated class does not have an `__init__` method, the decorated
class should define an `__init__` which accepts kwargs to set field values,
and raises appropriate arguments when other values are passed.
"""
"""If the decorated class does not have an `__init__` method, the decorated class
should define an `__init__` which accepts kwargs to set field values, and raises
appropriate arguments when other values are passed."""

x = ExampleComponentClass(a=2)
assert x.a == 2
Expand Down Expand Up @@ -157,10 +150,8 @@ class GrandParent:


def test_configure_automatically_instantiate_subcomponent():
"""
If there is only a single component subclass of a field type, an instance of
the class should be automatically instantiated during configuration.
"""
"""If there is only a single component subclass of a field type, an instance of the
class should be automatically instantiated during configuration."""

class AbstractChild:
pass
Expand Down Expand Up @@ -197,10 +188,8 @@ class Child2(AbstractChild):


def test_configure_non_interactive_missing_field_value(ExampleComponentClass):
"""
When not configuring interactively, an error should be raised if a field has
neither a default nor a configured value.
"""
"""When not configuring interactively, an error should be raised if a field has
neither a default nor a configured value."""

with pytest.raises(
ValueError,
Expand All @@ -210,10 +199,8 @@ def test_configure_non_interactive_missing_field_value(ExampleComponentClass):


def test_configure_interactive_prompt_missing_field_value(ExampleComponentClass):
"""
When configuring interactively, fields without default or configured values
should prompt for value input through the CLI.
"""
"""When configuring interactively, fields without default or configured values
should prompt for value input through the CLI."""

x = ExampleComponentClass()
a_value = 42
Expand All @@ -227,11 +214,9 @@ def test_configure_interactive_prompt_missing_field_value(ExampleComponentClass)


def test_configure_interactive_prompt_for_subcomponent_choice():
"""
When configuring interactively, sub-component fields without default or
configured values should prompt for a choice of subcomponents to instantiate
through the CLI.
"""
"""When configuring interactively, sub-component fields without default or
configured values should prompt for a choice of subcomponents to instantiate through
the CLI."""

class AbstractChild:
pass
Expand Down Expand Up @@ -278,10 +263,8 @@ class Parent:


def test_str_and_repr():
"""
`__str__` and `__repr__` should give formatted strings that represent nested
components nicely.
"""
"""`__str__` and `__repr__` should give formatted strings that represent nested
components nicely."""

@component
class Child1:
Expand Down Expand Up @@ -646,6 +629,85 @@ class Parent:
assert instance.child.a == 5


def test_component_configure_override():
@component
class A:
def __configure__(self, conf, **kwargs):
pass
jneeven marked this conversation as resolved.
Show resolved Hide resolved

# It doesn't call `configure`, so an error should be thrown.
with pytest.raises(
ValueError,
match=re.escape("`A` remains unconfigured after calling __configure__!"),
):
instance = A()
instance.__configure__({})

# This should pass
@component
class B:
attribute: int = Field(0)

def __configure__(self, *args, **kwargs):
self.attribute = 3
configure(self, *args, **kwargs)

instance = B()
instance.__configure__({})
assert instance.attribute == 3

# This also should not, since it isn't a function at all
with pytest.raises(
TypeError,
match=re.escape(
"The `__configure__` attribute of a @component class must be a method."
),
):

jneeven marked this conversation as resolved.
Show resolved Hide resolved
@component
class C:
__configure__ = "test"

instance = C()

# This should definitely pass, since it's the default
@component
class D:
test: str = Field("test")
pass

instance = D()
instance.__configure__({})

# This should not pass, since its arguments are wrong
with pytest.raises(
TypeError,
match=re.escape(
"The `__configure__` method of a @component class must match the "
"arguments of `configure()`"
),
):

jneeven marked this conversation as resolved.
Show resolved Hide resolved
@component
class E:
def __configure__(self, *args, extra=0):
pass

# This should not pass, since its arguments are wrong
with pytest.raises(
TypeError,
match=re.escape(
"The `__configure__` method of a @component class must match the "
"arguments of `configure()`"
),
):

@component
class F:
def __configure__(self, test, config):
pass


def test_component_pre_configure_setattr():
@component
class A:
Expand Down
9 changes: 3 additions & 6 deletions zookeeper/core/factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,8 @@


def _wrap_build(factory_cls: Type) -> None:
"""
Every @factory has a `build()` method, which we wrap so that `build()` is
only called once (lazily) and the value is cached.
"""
"""Every @factory has a `build()` method, which we wrap so that `build()` is only
called once (lazily) and the value is cached."""
fn = factory_cls.build

@functools.wraps(fn)
Expand Down Expand Up @@ -51,8 +49,7 @@ def wrapped_repr(factory_instance):


def factory(cls: Type):
"""
A decorator which turns a class into a Zookeeper factory.
"""A decorator which turns a class into a Zookeeper factory.

Factories are in particular Zookeeper components, so can have `Field`s and
`ComponentFields`. Factories must define an argument-less `build()` method,
Expand Down
6 changes: 3 additions & 3 deletions zookeeper/core/factory_registry.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"""
Keep track of user-defined @factory classes. This is defined here only to avoid
import loops.
"""Keep track of user-defined @factory classes.

This is defined here only to avoid import loops.
"""

from typing import Dict, Set, Type
Expand Down
8 changes: 3 additions & 5 deletions zookeeper/core/field.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,8 @@


class Field(Generic[C, F]):
"""
A configurable field for Zookeeper components. Fields must be typed, may
take default values, and are configurable through the CLI.
"""A configurable field for Zookeeper components. Fields must be typed, may take
default values, and are configurable through the CLI.

This class is not appropriate for nesting child sub-components; for this,
use `ComponentField` instead.
Expand Down Expand Up @@ -176,8 +175,7 @@ def get_default(self, instance: C) -> F:


class ComponentField(Field, Generic[C, F]):
"""
A Zookeeper field for nesting child sub-components.
"""A Zookeeper field for nesting child sub-components.

`ComponentField`s must be defined with a type annotation: a Python class
from which all possible component instances for the field must inherit.
Expand Down
3 changes: 1 addition & 2 deletions zookeeper/core/partial_component.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,7 @@


class PartialComponent(Generic[_ComponentType]):
"""
A wrapper around a component class that represents the component with some
"""A wrapper around a component class that represents the component with some
default field values modified, similar in principle to `functools.partial`.

`PartialComponent(SomeComponentClass, a=3)(b=4)` is equivalent to
Expand Down
Loading