Fix multilayer relative imports

[will-0]

Originally mentioned in this issue on the linkml repo.

When you attempt to use relative imports from a file that, itself, has been imported with a relative import, path resolution fails. This bug occurs both in linkml and linkml-runtime. In linkml-runtime, the problem occurs in schemaview.py:

if sn.startswith('.') and ':' not in i:
       i = os.path.normpath(str(Path(sn).parent / i))

If both sn and i are relative file paths, the path added to todo is an absolute path (at least on Mac). When i later than becomes sn, it's not picked up by the above clause and anything it imports does not hit the above condition and therefore just defined relative to the base file.

Marking as draft. @sneakers-the-rat noticed you were the last to edit this section, specifically here. Would appreciate input as to whether there's a reason for the above / it is expected behaviour and I'm missing something.

[sneakers-the-rat]

can you add a set of minimal schemas that demonstrates the bug to the tests?

see:

linkml-runtime/tests/test_utils/test_schemaview.py

Line 521 in a887aec

def test_imports_relative():

https://github.com/linkml/linkml-runtime/tree/main/tests/test_utils/input/imports_relative

[sneakers-the-rat]

related to: #350

[will-0]

@sneakers-the-rat test added that fails with original implementation, and fix that passes all tests.

Noted that some other work has been done to allow relative imports without './' prefix in #350. Can try and reconcile with the above if necessary.

[sneakers-the-rat]

seems fine, does need to be synchronized with the other changes that are being made to this exact place tho :)

[sneakers-the-rat]

why strip out the comment?

[will-0]

Have put this back in.

[sneakers-the-rat]

what is this doing?

[will-0]

Nothing, apologies! Was purely for inspection during debugging, meant to remove.

[sneakers-the-rat]

mild suggest:

Suggested change

	else:
	temp = i
	i = resolve_import(sn, i)
	temp = i
	i = resolve_import(sn, i)

everything after

if (condition):
    continue

is an else, but avoids needing to go 6 levels of indentation if we can avoid it

[will-0]

Thanks, have included this.

[sneakers-the-rat]

if we're splitting this out to a separate function, it would be great to not have the arguments be named sn and i

[will-0]

Have renamed to source_sch and imported_sch

[sneakers-the-rat]

Suggested change

	def resolve_import(sn: str, i: str) -> str:
	def _resolve_import(sn: str, i: str) -> str:

unless we expect this to be used elsewhere, probably make it private. it seems pretty specific to this one case in schemaview

[will-0]

Have done, thank you.

[sneakers-the-rat]

since we are already treating everything that isn't absolute or has a scheme as a path, i think this would be double-fixing the problem. e.g. tests pass with this

Suggested change

	path = os.path.normpath(str(Path(sn).parent / i))
	if i.startswith(".") and not path.startswith("."):
	# Above condition handles cases where both sn and i are relative paths, and path is not already relative
	return f"./{path}"
	else:
	return path
	return os.path.normpath(str(Path(sn).parent / i))

[will-0]

This was here to retain compatibility with hard-coded paths in test_imports_relative. If removed, following error raised:

test_utils/test_schemaview.py::test_imports_relative failed: def test_imports_relative():
...
>       assert closure == [
            'linkml:types',
            '../neighborhood_parent',
            'neighbor',
            '../parent',
            '../L1_0_1/L2_0_1_0/grandchild',
            '../../L0_1/L1_1_0/L2_1_0_0/apple',
            '../../L0_1/L1_1_0/L2_1_0_0/index',
            '../../L0_1/L1_1_0/L2_1_0_1/banana',
            '../../L0_1/L1_1_0/L2_1_0_1/index',
            '../../L0_1/L1_1_0/index',
            '../../L0_1/cousin',
            '../L1_0_1/dupe',
            './L2_0_0_0/child',
            './L2_0_0_1/child',
            'main'
        ]
E       AssertionError: assert ['linkml:type...0/apple', ...] == ['linkml:type...0/apple', ...]
E         
E         At index 12 diff: 'L2_0_0_0/child' != './L2_0_0_0/child'
E         Use -v to get more diff

[sneakers-the-rat]

ah yes. I do think it's useful to have all paths, internally in the imports_closure method, have a ./ or ../ in them so we can determine which need directory traversal trivially, but also acknowledge that would probably be done better with proper typing rather than string conventions and my prior impl sucks. Maybe changing the comment above to something like "ensure any path that requires directory traversal has a ./ prepended" or something like that would be good, but since the above is so anticipatory of future needs (e.g. resolving remote imports, resolving content-ID imports, other contexts where we need to differentiate posix-like directories from other formats) that it borders on YAGNI so i could go either way on what happens with this. thx for explaining

[will-0]

@sneakers-the-rat thanks for the review on the above, sorry it wasn't the cleanest (written prior to running off to a long string of night shifts!). Have synchronised with upstream and addressed your comments.

[amc-corey-cox]

This looks good to me.

[amc-corey-cox]

I'm going to wait to merge this until we land #345 to make sure we don't create any merge conflicts.

[ialarmedalien]

@will-0 Please make sure that you look at other pull requests to ensure that you're not fixing a bug that has already been fixed in another PR.

[will-0]

@ialarmedalien certainly, will make sure I do so more thoroughly in the future. Do you have the PR you're referring to? Did search up issues / PRs before but didn't see one at the time.

[sneakers-the-rat]

i think they're referring to this, i think they end up being equivalent?
#350