Improve custom_implementations algorithm further and add more tests

Author: LourensVeen

ymmsl/v0_2/resolver.py

@@ -158,10 +158,11 @@ def resolve_impls(
     rename_local_impls(config.programs, module, ylocals)
     rename_local_impls(config.models, module, ylocals)
     resolve_impl_imports(config, ylocals, ctx)
-    update_local_implementations(config, ylocals)
     config.imports = [i for i in config.imports if i.kind != ImportKind.IMPLEMENTATION]
 
-    return apply_custom_implementations(config, module, ylocals, ctx)
+    overwritten_impls = apply_custom_implementations(config, module, ylocals, ctx)
+    update_local_implementations(config, ylocals)
+    return overwritten_impls
 
 
 T = TypeVar('T', bound='Implementation')
@@ -225,16 +226,6 @@ def resolve_impl_imports(
         ctx.pop_import()
 
 
-def update_local_implementations(
-        config: Configuration, ylocals: Dict[Reference, Reference]) -> None:
-    """Updates names of local implementations to their full names."""
-    for model in config.models.values():
-        for cmp in model.components.values():
-            if cmp.implementation:
-                if cmp.implementation in ylocals:
-                    cmp.implementation = ylocals[cmp.implementation]
-
-
 def apply_custom_implementations(
         config: Configuration, module: Reference, ylocals: Dict[Reference, Reference],
         ctx: ResolutionContext) -> Set[Reference]:
@@ -268,6 +259,8 @@ def impl_hint_msg(
     overwritten_implementations = set()
     copied_paths = set()
 
+    # Pre-copy any models that will be updated, if they were imported and we therefore
+    # cannot modify them in place without interfering with other uses of the same model.
     for key, value in config.custom_implementations.items():
         base_model_name = Reference([key[0]])
         if ylocals.get(base_model_name) not in config.models:
@@ -282,9 +275,6 @@ def impl_hint_msg(
                     f'    Unknown implementation "{value}" in custom_implementations'
                     f' "{key}: {value}". {impl_hint_msg(value)}')
 
-        path = key[1:]
-        new_impl = ylocals[value] if value is not None else None
-
         # Before modifying it, we copy the model from its original a.b.c.Model to
         # <module>.Model so that the changes don't affect other uses of it, or the
         # cached version.
@@ -299,8 +289,13 @@ def impl_hint_msg(
             config.models[new_name] = m
             ylocals[base_model_name] = m.name
             overwritten_implementations.add(orig_name)
-        else:
-            m = orig_model
+
+    for key, value in config.custom_implementations.items():
+        base_model_name = Reference([key[0]])
+        path = key[1:]
+        new_impl = ylocals[value] if value is not None else None
+
+        m = config.models[ylocals[base_model_name]]
 
         # Now we can walk down the components and copy-and-rename the models along the
         # path, again to avoid making unintended changes elsewhere
@@ -319,6 +314,8 @@ def impl_hint_msg(
                         f' which does not have a component named {str(component)}.')
 
             orig_impl = m.components[component].implementation
+            if orig_impl in ylocals:
+                orig_impl = ylocals[orig_impl]
             if orig_impl is None or orig_impl not in config.models:
                 raise RuntimeError(
                         ctx.trace() +
@@ -372,6 +369,16 @@ def impl_hint_msg(
     return overwritten_implementations
 
 
+def update_local_implementations(
+        config: Configuration, ylocals: Dict[Reference, Reference]) -> None:
+    """Updates names of local implementations to their full names."""
+    for model in config.models.values():
+        for cmp in model.components.values():
+            if cmp.implementation:
+                if cmp.implementation in ylocals:
+                    cmp.implementation = ylocals[cmp.implementation]
+
+
 def find_impls(
         config: Configuration, name: Reference, ctx: ResolutionContext
         ) -> List[Implementation]:

ymmsl/v0_2/tests/test_resolver.py

@@ -17,6 +17,8 @@
 else:
     from importlib.metadata import EntryPoints, EntryPoint
 
+Ref = Reference
+
 
 @pytest.fixture
 def env_ymmsl_path() -> Generator[None, None, None]:
@@ -199,6 +201,137 @@ def test_apply_custom_implementations_set_none(env_ymmsl_path: None) -> None:
     assert model.components[Reference('micro')].implementation is None
 
 
+def test_apply_custom_implementations_no_hidden_copies() -> None:
+    ymmsl = (
+            'ymmsl_version: v0.2\n'
+            'description: |\n'
+            '  Testing that all local references point to the same object\n'
+            'models:\n'
+            '  A:\n'
+            '    description: Model A\n'
+            '    components:\n'
+            '      c1:\n'
+            '        ports: {}\n'
+            '        description: Component c1\n'
+            '  B:\n'
+            '    description: Model B\n'
+            '    components:\n'
+            '      c2:\n'
+            '        ports: {}\n'
+            '        description: Component c2\n'
+            '      c3:\n'
+            '        ports: {}\n'
+            '        description: Component c3\n'
+            'programs:\n'
+            '  p:\n'
+            '    ports:\n'
+            '    description: A program\n'
+            '    executable: /home/user/p\n'
+            )
+
+    config = load(ymmsl)
+    assert isinstance(config, Configuration)
+
+    config.models[Ref('A')].components[Ref('c1')].implementation = Ref('p')
+    config.custom_implementations[Reference('B.c2')] = Reference('A')
+    config.custom_implementations[Reference('B.c3')] = Reference('A')
+
+    resolve(Reference('no_copies'), config)
+
+
+    # Same thing but using custom implementations for everything
+    config = load(ymmsl)
+    assert isinstance(config, Configuration)
+
+    config.custom_implementations[Reference('A.c1')] = Reference('p')
+    config.custom_implementations[Reference('B.c2')] = Reference('A')
+    config.custom_implementations[Reference('B.c3')] = Reference('A')
+
+    resolve(Reference('no_copies'), config)
+
+    b = config.models[Reference('no_copies.B')]
+    assert b.components[Reference('c2')].implementation == 'no_copies.A'
+    assert b.components[Reference('c3')].implementation == 'no_copies.A'
+
+    a = config.models[Reference('no_copies.A')]
+    assert a.components[Reference('c1')].implementation == 'no_copies.p'
+
+    # this should yield the same result as above, because all references to A point to
+    # the same object
+    config = load(ymmsl)
+    assert isinstance(config, Configuration)
+
+    config.custom_implementations[Reference('B.c2')] = Reference('A')
+    config.custom_implementations[Reference('B.c3')] = Reference('A')
+    config.custom_implementations[Reference('A.c1')] = Reference('p')
+
+    resolve(Reference('no_copies2'), config)
+
+    b = config.models[Reference('no_copies2.B')]
+    assert b.components[Reference('c2')].implementation == 'no_copies2.A'
+    assert b.components[Reference('c3')].implementation == 'no_copies2.A'
+
+    a = config.models[Reference('no_copies2.A')]
+    assert a.components[Reference('c1')].implementation == 'no_copies2.p'
+
+
+def test_apply_custom_implementations_everything_localised() -> None:
+    ymmsl = (
+            'ymmsl_version: v0.2\n'
+            'description: |\n'
+            '  Testing that local and imported models are treated the same when\n'
+            '  customised.\n'
+            'imports:\n'
+            '- from a.e import implementation test_model\n'
+            'models:\n'
+            '  A:\n'
+            '    description: Model A\n'
+            '    components:\n'
+            '      c1:\n'
+            '        ports: {}\n'
+            '        description: Component c1\n'
+            '  B:\n'
+            '    description: Model B\n'
+            '    components:\n'
+            '      macro:\n'
+            '        ports: {}\n'
+            '        description: Component c2\n'
+            'programs:\n'
+            '  p:\n'
+            '    ports:\n'
+            '    description: A program\n'
+            '    executable: /home/user/p\n'
+            )
+
+    config = load(ymmsl)
+    assert isinstance(config, Configuration)
+
+    config.custom_implementations[Ref('A.c1')] = Ref('test_model')
+    config.custom_implementations[Ref('test_model.macro')] = Ref('p')
+
+    resolve(Reference('el'), config)
+
+    c1 = config.models[Ref('el.A')].components[Ref('c1')]
+    assert c1.implementation == 'el.test_model'
+
+    test_model = config.models[Ref('el.test_model')]
+    assert test_model.components[Ref('macro')].implementation == 'el.p'
+
+    config = load(ymmsl)
+    assert isinstance(config, Configuration)
+
+    config.custom_implementations[Ref('A.c1')] = Ref('B')
+    config.custom_implementations[Ref('B.macro')] = Ref('p')
+
+    resolve(Reference('el'), config)
+
+    c1 = config.models[Ref('el.A')].components[Ref('c1')]
+    assert c1.implementation == 'el.B'
+
+    b = config.models[Ref('el.B')]
+    assert b.components[Ref('macro')].implementation == 'el.p'
+
+
 def test_apply_custom_implementations_errors(env_ymmsl_path: None) -> None:
     ymmsl = (
             'ymmsl_version: v0.2\n'