linkml / linkml-runtime

Runtime support for linkml generated models
https://linkml.io/linkml/
Creative Commons Zero v1.0 Universal
24 stars 23 forks source link

Fix `Schemaview.import_closure` order #302

Closed sneakers-the-rat closed 7 months ago

sneakers-the-rat commented 7 months ago

Fix: https://github.com/linkml/linkml/issues/1839 Also:

Previously on this issue....

the schemaview was doing an ordering that was almost perfectly backwards, not allowing one to redefine classes that had already been declared in an imported schema.

Fix Ordering

Two substantive changes:

1) use a deque and appendleft

(probably a perf wash vs. reverse but wanted to keep operation grouped): the pop and append operations make a FILO queue, and so we want earlier visited items (eg. the imported module) to be at the end of the imports closure.

For intuition, in the sample import tree, the first layer of imports is

imports:
- linkml:types
- s1
- s2
- s3

so the todo list will have ['linkml:types', 's1', 's2', 's3']

on the next iteration we

and so on.

2) only check for uniqueness to avoid cycles

remove uniqueness checking in closure appends, and add to visited at the end of the loop rather than the start.

This is mostly to fix the case where a schema (like linkml:types) is imported multiple times in the tree. Currently, it would be inserted in the last place it appears (since the import resolution order effectively works backwards). In this change we add it everywhere that it is actually imported, but only follow its imports the first time it appears. This is an imperfect fix that comes from needing to flatten a graph that can be completed once the schemaview class behaved recursively.

We then filter duplicates, keeping the first appearance using a dictionary comprehension.

Also Changed

traverse param:

I noticed that the traverse parameter behaved identically to the imports parameter, but i could only see the imports parameter being used. I switched that to being default None so it would only to a deprecation warning if you attempted to use it. I don't know how y'all handle deprecations so lmk how to handle that.

issue 998 tests

I'm not sure why I affected this here, but this test was incorrectly specifying that there was only one slot that used 'EmploymentStatusEnum' (which actually doesn't exist in the schema, it's named 'EmployedStatusEnum', so maybe schemaview needs some tests for whether a thing exists when referring to it?), but there are actually two slots that do:

And since there are actually two employed attributes, there should really be 3 slots identified there, but since they are just indexed by slot name they are collapsed down to 2.

idk i didn't want to change too much there, but that's sort of a warning sign that there might be something wrong elsewhere in schemaview, but i changed it so the tests correctly pass, not sure why they were passing before.

codecov[bot] commented 7 months ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (2e35728) 62.90% compared to head (1b2ac02) 62.89%. Report is 7 commits behind head on main.

Files Patch % Lines
linkml_runtime/utils/schemaview.py 78.57% 1 Missing and 2 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #302 +/- ## ========================================== - Coverage 62.90% 62.89% -0.01% ========================================== Files 62 62 Lines 8529 8530 +1 Branches 2239 2240 +1 ========================================== Hits 5365 5365 Misses 2554 2554 - Partials 610 611 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.