Merge pull request #302 from sneakers-the-rat/bugfix-schemaview-import-order

Fix `Schemaview.import_closure` order

Author: cmungall

linkml_runtime/utils/schemaview.py

@@ -4,9 +4,10 @@
 import collections
 from functools import lru_cache
 from copy import copy, deepcopy
-from collections import defaultdict
+from collections import defaultdict, deque
 from pathlib import Path
 from typing import Mapping, Tuple
+import warnings
 
 from linkml_runtime.utils.namespaces import Namespaces
 from deprecated.classic import deprecated
@@ -207,34 +208,76 @@ def load_import(self, imp: str, from_schema: SchemaDefinition = None):
         return schema
 
     @lru_cache()
-    def imports_closure(self, imports: bool = True, traverse=True, inject_metadata=True) -> List[SchemaDefinitionName]:
+    def imports_closure(self, imports: bool = True, traverse: Optional[bool] = None, inject_metadata=True) -> List[SchemaDefinitionName]:
         """
         Return all imports
 
-        :param traverse: if true, traverse recursively
+        Objects in imported classes override one another in a "python-like" order -
+        from the point of view of the importing schema, imports will override one
+        another from first to last, recursively for each layer of imports.
+
+        An import tree like::
+
+            - main
+              - s1
+                - s1_1
+                - s1_2
+                    - s1_2_1
+                    - s1_2_2
+              - s2
+                - s2_1
+                - s2_2
+
+        will override objects with the same name, in order::
+
+            ['s1_1', 's1_2_1', 's1_2_2', 's1_2', 's1', 's2_1', 's2_2', 's2']
+
+        :param imports: bool (default: ``True`` ) include imported schemas, recursively
+        :param traverse: bool, optional (default: ``True`` ) (Deprecated, use
+            ``imports`` ). if true, traverse recursively
         :return: all schema names in the transitive reflexive imports closure
         """
-        if not imports:
-            return [self.schema.name]
         if self.schema_map is None:
             self.schema_map = {self.schema.name: self.schema}
-        closure = []
+
+        closure = deque()
         visited = set()
         todo = [self.schema.name]
-        if not traverse:
+
+        if traverse is not None:
+            warnings.warn(
+                'traverse behaves identically to imports and will be removed in a future version. Use imports instead.',
+                DeprecationWarning
+            )
+
+        if not imports or (not traverse and traverse is not None):
             return todo
+
         while len(todo) > 0:
+            # visit item
             sn = todo.pop()
-            visited.add(sn)
             if sn not in self.schema_map:
                 imported_schema = self.load_import(sn)
                 self.schema_map[sn] = imported_schema
-            s = self.schema_map[sn]
-            if sn not in closure:
-                closure.append(sn)
-            for i in s.imports:
-                if i not in visited:
-                    todo.append(i)
+
+            # resolve item's imports if it has not been visited already
+            # we will get duplicates, but not cycles this way, and
+            # filter out dupes, preserving the first entry, at the end.
+            if sn not in visited:
+                for i in self.schema_map[sn].imports:
+                    # no self imports ;)
+                    if i != sn:
+                        todo.append(i)
+
+            # add item to closure
+            # append + pop (above) is FILO queue, which correctly extends tree leaves,
+            # but in backwards order.
+            closure.appendleft(sn)
+            visited.add(sn)
+
+        # filter duplicates, keeping first entry
+        closure = list({k:None for k in closure}.keys())
+
         if inject_metadata:
             for s in self.schema_map.values():
                 for x in {**s.classes, **s.enums, **s.slots, **s.subsets, **s.types}.values():
@@ -434,6 +477,7 @@ def all_elements(self, imports=True) -> Dict[ElementName, Element]:
     def _get_dict(self, slot_name: str, imports=True) -> Dict:
         schemas = self.all_schema(imports)
         d = {}
+        # pdb.set_trace()
         # iterate through all schemas and merge the list together
         for s in schemas:
             # get the value of element name from the schema, if empty, return empty dictionary.

tests/test_issues/test_linkml_issue_998.py

@@ -81,8 +81,8 @@ def test_slots_are_not_duplicated(view):
 
 def test_issue_998_attribute_slot(view):
     enum_slots = view.get_slots_by_enum("EmploymentStatusEnum")
-    assert len(enum_slots) == 1
-    assert enum_slots[0].name == "employed"
+    assert len(enum_slots) == 2
+    assert sorted([slot.name for slot in enum_slots]) == ["employed", "past_employer"]
 
 
 def test_issue_998_schema_and_attribute_slots(view):

tests/test_utils/input/imports/README.md

@@ -0,0 +1,33 @@
+A tree of imports like:
+
+```
+main
+|- linkml:types
+|- s1
+|  |- s1_1
+|  |- s1_2
+|     |- s1_2_1
+|        |- s1_2_1_1
+|           |- s1_2_1_1_1
+|           |- s1_2_1_1_2
+|- s2
+|  |- s2_1
+|  |- s2_2
+|- s3
+   |- s3_1
+   |- s3_2
+```
+
+This is used to test SchemaView's logic for complex import hierarchies,
+eg.
+- overrides
+- imports closure completeness
+- (at some point) monotonicity
+- etc. 
+
+Currently, each schema...
+- Contains one `Main` class with a value whose default is overridden to indicate which module defined it, this is used to test overrides.
+- Contains a class like `S2` that carries the name of the module to ensure that unique classes are gotten from the whole tree
+- Each node in the tree outwards from `main` will carry the 'special classes' from the parents, overriding them as with `main` to
+  get a more detailed picture of override order: eg. `s1_2_1` will also define `S1_2` and override its `ifabsent` field, 
+  which `S1_2` should override since it's the importing schema.

tests/test_utils/input/imports/main.yaml

@@ -0,0 +1,15 @@
+id: main
+name: main
+title: main
+imports:
+  - linkml:types
+  - s1
+  - s2
+  - s3
+classes:
+  Main:
+    description: "The class we use to test overrides on imports!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "Main"

tests/test_utils/input/imports/s1.yaml

@@ -0,0 +1,20 @@
+id: s1
+name: s1
+title: s1
+imports:
+  - linkml:types
+  - s1_1
+  - s1_2
+classes:
+  Main:
+    description: "The class we use to test overrides on imports!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1"
+  S1:
+    description: "A class from one of the imported classes!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1"

tests/test_utils/input/imports/s1_1.yaml

@@ -0,0 +1,24 @@
+id: s1_1
+name: s1_1
+title: s1_1
+imports:
+  - linkml:types
+classes:
+  Main:
+    description: "The class we use to test overrides on imports!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_1"
+  S1:
+    description: "A class from one of the imported classes!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_1"
+  S1_1:
+    description: "A class from one of the imported classes!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_1"

tests/test_utils/input/imports/s1_2.yaml

@@ -0,0 +1,25 @@
+id: s1_2
+name: s1_2
+title: s1_2
+imports:
+  - linkml:types
+  - s1_2_1
+classes:
+  Main:
+    description: "The class we use to test overrides on imports!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_2"
+  S1:
+    description: "A class from one of the imported classes!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_2"
+  S1_2:
+    description: "A class from one of the imported classes!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_2"

tests/test_utils/input/imports/s1_2_1.yaml

@@ -0,0 +1,31 @@
+id: s1_2_1
+name: s1_2_1
+title: s1_2_1
+imports:
+  - linkml:types
+  - s1_2_1_1
+classes:
+  Main:
+    description: "The class we use to test overrides on imports!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_2_1"
+  S1:
+    description: "A class from one of the imported classes!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_2_1"
+  S1_2:
+    description: "A class from one of the imported classes!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_2_1"
+  S1_2_1:
+    description: "A class from one of the imported classes!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_2_1"

tests/test_utils/input/imports/s1_2_1_1.yaml

@@ -0,0 +1,38 @@
+id: s1_2_1_1
+name: s1_2_1_1
+title: s1_2_1_1
+imports:
+  - linkml:types
+  - s1_2_1_1_1
+  - s1_2_1_1_2
+classes:
+  Main:
+    description: "The class we use to test overrides on imports!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_2_1_1"
+  S1:
+    description: "A class from one of the imported classes!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_2_1_1"
+  S1_2:
+    description: "A class from one of the imported classes!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_2_1_1"
+  S1_2_1:
+    description: "A class from one of the imported classes!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_2_1_1"
+  S1_2_1_1:
+    description: "A class from one of the imported classes!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_2_1_1"

tests/test_utils/input/imports/s1_2_1_1_1.yaml

@@ -0,0 +1,42 @@
+id: s1_2_1_1_1
+name: s1_2_1_1_1
+title: s1_2_1_1_1
+imports:
+  - linkml:types
+classes:
+  Main:
+    description: "The class we use to test overrides on imports!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_2_1_1_1"
+  S1:
+    description: "A class from one of the imported classes!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_2_1_1_1"
+  S1_2:
+    description: "A class from one of the imported classes!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_2_1_1_1"
+  S1_2_1:
+    description: "A class from one of the imported classes!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_2_1_1_1"
+  S1_2_1_1:
+    description: "A class from one of the imported classes!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_2_1_1_1"
+  S1_2_1_1_1:
+    description: "A class from one of the imported classes!"
+    attributes:
+      value:
+        range: string
+        ifabsent: "S1_2_1_1_1"