Warn on unused arguments (#238)

* add more detail to error message

* Raise warning on partial template eval

Adds a new flag, warn_unused, to Template.evaluate; defaults to True.
This raises a warning if not all of the mandatory parameters are
provided to the Template when it is evaluated. Will raise warning
if optional arguments are required (when using the corresponding flag on
Template.evaluate)

* Add tests for raising warnings when missing params on Template.evaluate

* add a pytest marker for integration tests

* fix behavior around inlining dependencies with optional args; add tests

* add fixtures for tests

* Update buildingmotif/dataclasses/template.py

Co-authored-by: Tobias Shapinsky <[email protected]>

* fix another edge case in inlining dependencies with optional args; add tests

* Fix 223P dependencies in the templates

Due to how we have the dependencies structured, the mapsTo relationship
needs to refer to a connection point that lies in a dependency's
dependency. This makes that relationship explicit

* Split template dep mgmt into two phases

New problem is when we are testing the libraries for validity,
sometimes we want to give an explicit argument to the 'mapsto'
optional arg w/n a dependency. However, we currently can't give
a binding to a nested dependency. The result is that we end up with
multiple mapsto (because the 'optional' mapsto argument gets a newly
minted URI from template.fill()), which fails validation.

This commit splits template dependency management into 2 phases: an
initial phase which adds the dependency links and args to the DB; a
second phase which checks that the args/params between the template and
its dependencies are all valid and work as expected.

* add flag to force adding optional_args when calling fill()

* don't treat SHACL warnings as violations

* adjust notebook to require optional args for 223P; add test to be sure

* Update buildingmotif/database/table_connection.py

Co-authored-by: Matt Steen <[email protected]>

* Update buildingmotif/database/table_connection.py

Co-authored-by: Matt Steen <[email protected]>

* Update buildingmotif/utils.py

Co-authored-by: Matt Steen <[email protected]>

* Update buildingmotif/database/table_connection.py

Co-authored-by: Matt Steen <[email protected]>

* Update buildingmotif/database/table_connection.py

Co-authored-by: Matt Steen <[email protected]>

* Update buildingmotif/utils.py

Co-authored-by: Matt Steen <[email protected]>

* Update buildingmotif/utils.py

Co-authored-by: Matt Steen <[email protected]>

* update method names + docstrings

* Update buildingmotif/dataclasses/template.py

* Update buildingmotif/utils.py

* add check-dependencies to tests

---------

Co-authored-by: Tobias Shapinsky <[email protected]>
Co-authored-by: Matt Steen <[email protected]>

buildingmotif/database/table_connection.py

@@ -1,5 +1,6 @@
 import logging
 import uuid
+from functools import lru_cache
 from typing import Dict, List, Tuple
 
 from sqlalchemy.engine import Engine
@@ -12,7 +13,6 @@
     DBTemplate,
     DepsAssociation,
 )
-from buildingmotif.utils import get_parameters
 
 
 class TableConnection:
@@ -377,10 +377,21 @@ def update_db_template_optional_args(
         )
         db_template.optional_args = optional_args
 
-    def add_template_dependency(
+    def add_template_dependency_preliminary(
         self, template_id: int, dependency_id: int, args: Dict[str, str]
     ):
-        """Create dependency between two templates.
+        """Creates a *preliminary* dependency between two templates. This dependency
+        is preliminary because the bindings between the dependent/dependency templates
+        are *not validated*. This serves to populate the directed acyclic graph of dependencies between templates
+        before the parameter bindings are checked. This ensures that all of the parameters for
+        a template *and those in its dependencies* can all be identified and used as part of
+        the parameter binding check. The upshot of this process is dependencies between two
+        templates can refer to parameters in a template *or its dependencies*. This is necessary
+        to support templates that contain many nested components that refer to each other (such
+        as the s223:mapsTo property).
+
+        **Important:** Be sure to run "check_all_template_dependencies" to ensure all of your templates.
+        will work as you expect!
 
         :param template_id: dependant template id
         :type template_id: int
@@ -390,56 +401,94 @@ def add_template_dependency(
         :type args: Dict[str, str]
         :raises ValueError: if all dependee required_params not in args
         :raises ValueError: if dependant and dependency template don't share a
-            library
         """
         self.logger.debug(
             f"Creating depencency from templates with ids: '{template_id}' and: '{dependency_id}'"
         )
         templ = self.get_db_template_by_id(template_id)
-        graph = self.bm.graph_connection.get_graph(templ.body_id)
-        params = get_parameters(graph)
-        dep = self.get_db_template_by_id(dependency_id)
+        if "name" not in args.keys():
+            raise ValueError(
+                f"The name parameter is required for the dependency '{templ.name}'."
+            )
+        # In the past we had a check here to make sure the two templates were in the same library.
+        # This has been removed because it wasn't actually necessary, but we may add it back in
+        # in the future.
+        relationship = DepsAssociation(
+            dependant_id=template_id,
+            dependee_id=dependency_id,
+            args=args,
+        )
+
+        self.bm.session.add(relationship)
+        self.bm.session.flush()
+
+    def check_all_template_dependencies(self):
+        """
+        Verifies that all template dependencies have valid references to the parameters
+        in the dependency or (recursively) its dependencies. Raises an exception if any
+        issues are found. Checks all templates in the database.
+        """
+        # TODO: maybe optimize this to only check recently added templates
+        # (i.e. maybe those added since last commit?)
+        for db_templ in self.get_all_db_templates():
+            for dep in self.get_db_template_dependencies(db_templ.id):
+                self.check_template_dependency_relationship(dep)
+
+    @lru_cache(maxsize=128)
+    def check_template_dependency_relationship(self, dep: DepsAssociation):
+        """Verify that the dependency between two templates is well-formed. This involves
+        a series of checks:
+        - existence of the dependent and dependency templates is performed during the add_ method
+        - the args keys appear in the dependency, or recursively in a template that is a dependency
+          of the named dependency
+        - the args values appear in the dependent template
+        - there is a 'name' parameter in the dependent template
+
+        :param dep: the dependency object to check
+        :type dep: DepsAssociation
+        :raises ValueError: if all dependee required_params not in args
+        :raises ValueError: if dependant and dependency template don't share a
+            library
+        """
+        template_id = dep.dependant_id
+        dependency_id = dep.dependee_id
+        args = dep.args
+        self.logger.debug(
+            f"Creating depencency from templates with ids: '{template_id}' and: '{dependency_id}'"
+        )
+        from buildingmotif.dataclasses import Template
+
+        templ = Template.load(template_id)
+        params = templ.inline_dependencies().parameters
+        dep_templ = Template.load(dependency_id)
+        dep_params = dep_templ.inline_dependencies().parameters
 
         # check parameters are valid
         if not set(args.values()).issubset(params):
             raise ValueError(
-                f"In {templ.name} the values of the bindings to {dep.name} must correspond to the "
+                f"In {templ.name} the values of the bindings to {dep_templ.name} must correspond to the "
                 "parameters in the dependant template."
-                f"Dependency {dep.name} refers to params {set(args.values()).difference(params)} "
-                f"that do not appear in template {templ.name}"
+                f"Dependency {dep_templ.name} refers to params {set(args.values()).difference(params)} "
+                f"that do not appear in template {templ.name} ({params})."
             )
         # do the same for the dependency
-        graph = self.bm.graph_connection.get_graph(dep.body_id)
-        dep_params = get_parameters(graph)
-        if not set(args.keys()).issubset(dep_params):
+        binding_params = set(args.keys())
+        if not binding_params.issubset(dep_params):
+            diff = binding_params.difference(dep_params)
             raise ValueError(
-                f"In {templ.name} the keys of the bindings to {dep.name} must correspond to the "
-                "parameters in the template dependency"
+                f"In {templ.name} the keys of the bindings to {dep_templ.name} must correspond to the "
+                "parameters in the template dependency. "
+                f"The dependency binding uses params {diff} which don't appear in the dependency template. "
+                f"Available dependency parameters are {dep_params}"
             )
 
         # TODO: do we need this kind of check?
-        if "name" not in args.keys():
-            raise ValueError(
-                f"The name parameter is required for the dependency '{templ.name}'"
-            )
         if len(params) > 0 and args["name"] not in params:
             raise ValueError(
                 "The name parameter of the dependency must be bound to a param in this template."
                 f"'name' was bound to {args['name']} but available params are {params}"
             )
 
-        # In the past we had a check here to make sure the two templates were in the same library.
-        # This has been removed because it wasn't actually necessary, but we may add it back in
-        # in the future.
-        relationship = DepsAssociation(
-            dependant_id=template_id,
-            dependee_id=dependency_id,
-            args=args,
-        )
-
-        self.bm.session.add(relationship)
-        self.bm.session.flush()
-
     def remove_template_dependency(self, template_id: int, dependency_id: int):
         """Remove dependency between two templates.
 

buildingmotif/dataclasses/library.py

@@ -18,10 +18,13 @@
 from buildingmotif.database.tables import DBLibrary, DBTemplate
 from buildingmotif.dataclasses.shape_collection import ShapeCollection
 from buildingmotif.dataclasses.template import Template
-from buildingmotif.namespaces import XSD
 from buildingmotif.schemas import validate_libraries_yaml
 from buildingmotif.template_compilation import compile_template_spec
-from buildingmotif.utils import get_ontology_files, get_template_parts_from_shape
+from buildingmotif.utils import (
+    get_ontology_files,
+    get_template_parts_from_shape,
+    skip_uri,
+)
 
 if TYPE_CHECKING:
     from buildingmotif import BuildingMOTIF
@@ -252,6 +255,7 @@ def _load_from_ontology(
             advanced=True,
             inplace=True,
             js=True,
+            allow_warnings=True,
         )
 
         lib = cls.create(ontology_name, overwrite=overwrite)
@@ -378,6 +382,8 @@ def _resolve_template_dependencies(
         template_id_lookup: Dict[str, int],
         dependency_cache: Mapping[int, Union[List[_template_dependency], List[dict]]],
     ):
+        # two phases here: first, add all of the templates and their dependencies
+        # to the database but *don't* check that the dependencies are valid yet
         for template in self.get_templates():
             if template.id not in dependency_cache:
                 continue
@@ -386,12 +392,8 @@ def _resolve_template_dependencies(
                     if dep["template"] in template_id_lookup:
                         dependee = Template.load(template_id_lookup[dep["template"]])
                         template.add_dependency(dependee, dep["args"])
-                    # Now that we have all the templates, we can populate the dependencies.
-                    # IGNORES missing XSD imports --- there is really no reason to import the XSD
-                    # ontology because the handling is baked into the software processing the RDF
-                    # graph. Thus, XSD URIs will always yield import warnings. This is noisy, so we
-                    # suppress them.
-                    elif not dep["template"].startswith(XSD):
+                    # check documentation for skip_uri for what URIs get skipped
+                    elif not skip_uri(dep["template"]):
                         logging.warn(
                             f"Warning: could not find dependee {dep['template']}"
                         )
@@ -402,6 +404,9 @@ def _resolve_template_dependencies(
                     except Exception as e:
                         logging.warn(f"Warning: could not resolve dependency {dep}")
                         raise e
+        # check that all dependencies are valid (use parameters that exist, etc)
+        for template in self.get_templates():
+            template.check_dependencies()
 
     def _read_yml_file(
         self,

buildingmotif/dataclasses/model.py

@@ -216,6 +216,7 @@ def compile(self, shape_collections: List["ShapeCollection"]):
             advanced=True,
             inplace=True,
             js=True,
+            allow_warnings=True,
         )
         post_compile_length = len(model_graph)  # type: ignore
 
@@ -229,6 +230,7 @@ def compile(self, shape_collections: List["ShapeCollection"]):
                 advanced=True,
                 inplace=True,
                 js=True,
+                allow_warnings=True,
             )
             post_compile_length = len(model_graph)  # type: ignore
             attempts -= 1