Repository: larq/zookeeper

Issue:
Error when configuring components with values inherited from factories
The following fails:

```python
    @factory
    class IntFactory:
        def build(self) -> int:
            return 5

    @component
    class Child:
        x: int = ComponentField()

    @component
    class Parent:
        child: Child = ComponentField(Child)
        x: int = ComponentField(IntFactory)

    p = Parent()
    configure(p, {})
    assert p.x == 5
    assert p.child.x == 5
```

with error `AttributeError: 'int' object has no attribute '__component_configured__'`.

This happens because `configure()` tries to recursively configure `p.child.x`, as it is a `ComponentField` (shadowing `p.x`, which is a `ComponentField`). But `p.child.x` resolves to an integer. We can solve this by checking that the thing we try and recursively configure is a component instance (this will not break type-checking, which happens separately).

Discussion:

Patch:
diff --git a/zookeeper/core/component.py b/zookeeper/core/component.py
index 8ce0517..7944fa6 100644
--- a/zookeeper/core/component.py
+++ b/zookeeper/core/component.py
@@ -106,7 +106,7 @@ def _type_check_and_cache(instance, field: Field, result: Any) -> None:
         raise TypeError(
             f"Field '{field.name}' of component '{instance.__component_name__}' is "
             f"annotated with type '{field.type}', which is not satisfied by "
-            f"default value {repr(result)}."
+            f"value {repr(result)}."
         )
 
     object.__setattr__(instance, field.name, result)
@@ -186,7 +186,7 @@ def _wrap_getattribute(component_cls: Type) -> None:
                 utils.generate_component_ancestors_with_field(instance, name), None
             )
             try:
-                result = getattr(parent_instance, name)
+                result = parent_instance.__base_getattribute__(name)  # type: ignore
             except AttributeError:
                 # From here we raise the original exception instead because it
                 # will correctly refer to this component rather than some parent
@@ -655,6 +655,9 @@ def configure(
                 continue
             raise e from None
 
+        if not utils.is_component_instance(sub_component_instance):
+            continue
+
         full_name = f"{instance.__component_name__}.{field.name}"
 
         if not sub_component_instance.__component_configured__:
diff --git a/zookeeper/tf/dataset.py b/zookeeper/tf/dataset.py
index fb79156..2a1053a 100644
--- a/zookeeper/tf/dataset.py
+++ b/zookeeper/tf/dataset.py
@@ -4,6 +4,7 @@ from typing import Dict, Optional, Tuple
 import tensorflow as tf
 import tensorflow_datasets as tfds
 
+from zookeeper.core import utils
 from zookeeper.core.field import Field
 
 
@@ -61,6 +62,9 @@ class TFDSDataset(Dataset):
     # The directory that the dataset is stored in.
     data_dir: Optional[str] = Field(None)
 
+    # Whether or not to download the dataset (if it's not already downloaded).
+    download: bool = Field(False)
+
     # Train and validation splits. A validation split is not required.
     train_split: str = Field()
     validation_split: Optional[str] = Field(None)
@@ -97,13 +101,23 @@ class TFDSDataset(Dataset):
     def load(self, split, decoders, shuffle) -> tf.data.Dataset:
         """Return a `tf.data.Dataset` object representing the requested split."""
 
-        return tfds.load(
-            name=self.name,
-            split=split,
-            data_dir=self.data_dir,
-            decoders=decoders,
-            as_dataset_kwargs={"shuffle_files": shuffle},
-        )
+        try:
+            return tfds.load(
+                name=self.name,
+                split=split,
+                data_dir=self.data_dir,
+                download=self.download,
+                decoders=decoders,
+                as_dataset_kwargs={"shuffle_files": shuffle},
+            )
+        except AssertionError as e:
+            if not self.download:
+                utils.warn(
+                    f"Field 'download' of component {self.__class__.__name__} is False. "
+                    "If the TFDS dataset is not downloaded, set 'download' to True to "
+                    "call `download_and_prepare()` automatically."
+                )
+            raise e from None
 
     def train(self, decoders=None) -> Tuple[tf.data.Dataset, int]:
         return (
@@ -129,6 +143,12 @@ class MultiTFDSDataset(Dataset):
     to be trained on data that is combined from multiple datasets.
     """
 
+    # The directory that the dataset is stored in.
+    data_dir: Optional[str] = Field(None)
+
+    # Whether or not to download the dataset (if it's not already downloaded).
+    download: bool = Field(False)
+
     # A non-empty mapping from dataset names as keys to splits as values. The
     # training data will be the concatenation of the datasets loaded from each
     # (key, value) pair.
@@ -138,9 +158,6 @@ class MultiTFDSDataset(Dataset):
     # empty, indicating no validation data.
     validation_split: Dict[str, str] = Field(lambda: {})
 
-    # The directory that the dataset is stored in.
-    data_dir: Optional[str] = Field(None)
-
     def num_examples(self, splits) -> int:
         """
         Compute the total number of examples in the splits specified by the
@@ -156,13 +173,23 @@ class MultiTFDSDataset(Dataset):
     def load(self, splits, decoders, shuffle) -> tf.data.Dataset:
         result = None
         for name, split in splits.items():
-            dataset = tfds.load(
-                name=name,
-                split=split,
-                data_dir=self.data_dir,
-                decoders=decoders,
-                as_dataset_kwargs={"shuffle_files": shuffle},
-            )
+            try:
+                dataset = tfds.load(
+                    name=name,
+                    split=split,
+                    data_dir=self.data_dir,
+                    download=self.download,
+                    decoders=decoders,
+                    as_dataset_kwargs={"shuffle_files": shuffle},
+                )
+            except AssertionError as e:
+                if not self.download:
+                    utils.warn(
+                        f"Field 'download' of component {self.__class__.__name__} is "
+                        "False. If the TFDS dataset is not downloaded, set 'download' "
+                        "to True to call `download_and_prepare()` automatically."
+                    )
+                raise e from None
             result = result.concatenate(dataset) if result is not None else dataset
         return result
 


Test patch:
diff --git a/.github/workflows/unittest.yml b/.github/workflows/unittest.yml
index 3da10d9..489e3fe 100644
--- a/.github/workflows/unittest.yml
+++ b/.github/workflows/unittest.yml
@@ -31,7 +31,7 @@ jobs:
       - name: Test with pytest
         run: pytest . -vv --cov=zookeeper --cov-report=xml --cov-config=.coveragerc
       - name: Test example script # Test that the model successfully compiles.
-        run: python examples/larq_experiment.py BinaryNetMnist epochs=0
+        run: python examples/larq_experiment.py BinaryNetMnist epochs=0 --dataset.download
       - name: Upload code-coverage report to Codecov
         if: matrix.python-version == '3.7' && matrix.tf-version == '2.0.1'
         run: curl -s https://codecov.io/bash | bash -s -- -t $token -f ./coverage.xml -F unittests
diff --git a/zookeeper/core/component_test.py b/zookeeper/core/component_test.py
index 8495571..0d188ba 100644
--- a/zookeeper/core/component_test.py
+++ b/zookeeper/core/component_test.py
@@ -326,7 +326,7 @@ def test_type_check(ExampleComponentClass):
     # Attempting to access the field should now raise a type error.
     with pytest.raises(
         TypeError,
-        match="Field 'a' of component 'x' is annotated with type '<class 'int'>', which is not satisfied by default value 4.5.",
+        match="Field 'a' of component 'x' is annotated with type '<class 'int'>', which is not satisfied by value 4.5.",
     ):
         instance.a
 
@@ -417,6 +417,34 @@ def test_component_getattr_value_via_factory_parent():
     assert f.build() == 5
 
 
+def test_component_inherited_factory_value():
+    """https://github.com/larq/zookeeper/issues/123."""
+
+    @factory
+    class IntFactory:
+        def build(self) -> int:
+            return 5
+
+    @component
+    class Child:
+        x: int = ComponentField()
+
+    @component
+    class Parent:
+        child: Child = ComponentField(Child)
+        x: int = ComponentField(IntFactory)
+
+    p = Parent()
+    configure(p, {})
+    assert p.x == 5
+    assert p.child.x == 5
+
+    p = Parent()
+    configure(p, {"child.x": 7})
+    assert p.x == 5
+    assert p.child.x == 7
+
+
 def test_component_post_configure():
     with pytest.raises(
         TypeError,