Fix load_morpholgies to always resolve paths

[eleftherioszisis]

Issue: If a relative morphology directory is passed into neurom.load_morphologies, a Population object is created with relative paths to the morphologies. If the directory is changed after the Population is created, the paths will not be valid anymore.
Changes: Population makes paths absolute. Note that it does not resolve symlinks.
Fixes #1046

[adrien-berchet]

I am wondering if the path resolution should not be located in the Population constructor? Because creating a new Population instance will still have the same issue if the user does not use the load_morphologies function.

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (v4@94f79e5). Click here to learn what that means.
The diff coverage is n/a.

@@          Coverage Diff           @@
##             v4     #1047   +/-   ##
======================================
  Coverage      ?   100.00%           
======================================
  Files         ?        35           
  Lines         ?      2475           
  Branches      ?         0           
======================================
  Hits          ?      2475           
  Misses        ?         0           
  Partials      ?         0           

[eleftherioszisis]

I am wondering if the path resolution should not be located in the Population constructor? Because creating a new Population instance will still have the same issue if the user does not use the load_morphologies function.

I am not so much in favor for the Population to do io stuff, as it is not its job.

[adrien-berchet]

I am not so much in favor for the Population to do io stuff, as it is not its job.

But it already does, isn't it? If the cache is disabled, only file paths are stored in the Population object.

[eleftherioszisis]

I am not so much in favor for the Population to do io stuff, as it is not its job.

But it already does, isn't it? If the cache is disabled, only file paths are stored in the Population object.

Well, yeah but my issue is that it can take either files or morphologies as the first argument. However, for the resolution to be correct, it cannot be lazy, because the directory can be changed at any time after init, before calling the iterators. So for this to work on the population, I would need to add a line that checks all files if they are morphologies or paths and then resolve them as needed, before letting cache and the rest do their own thing.

I would be more willing to add an assertion in Population._load_file for the path to be absolute. However, this is limiting if someone wants to create intentionally a population from relative paths.

[adrien-berchet]

Yeah, I prefer the first solution (resolving before the cache and other stuffs) because it makes it more consistent. But I agree that it would be cleaner to have a constructor that only accept a list of morphologies so the paths can be processed elsewhere but this would induce breaking changes. So as you prefer :-)

[eleftherioszisis]

Unfortunately, I cannot make Population fully resolve the paths, because there are test cases that use symlinks to morphologies with different filename...

Therefore paths are made absolute, but they are not resolved, which means that neither symlinks are resolved nor dot paths e.g. "..".

Edit: I have ended up resolving the parent, but not the full path. This works with the symlinked files and resolves any relative path or symlinks in the parent path.

[adrien-berchet]

I don't get what's the issue with symlinks to morphologies with different filename? Is it because the tests check the file names?

[eleftherioszisis]

I don't get what's the issue with symlinks to morphologies with different filename? Is it because the tests check the file names?

According to the test:

NeuroM/tests/io/test_io_utils.py

Lines 220 to 226 in 7ef31d9

	def test_load_morphologies_filenames():
	pop = utils.load_morphologies(FILENAMES, name='test123')
	assert len(pop) == 2
	assert pop.name == 'test123'
	for m, name in zip(pop.morphologies, NRN_NAMES):
	assert isinstance(m, Morphology)
	assert m.name == name

The name of the morphology should be the name of the file that is passed. If the symlink uses a different name and the path is resolved, that name will be lost to the name of the file it points to.

[adrien-berchet]

Hmmm ok
Is it really a feature (having morphology names of the population equal to the unresolved file name) or is it just something we check to be sure the test passed (in this case we could just change how we check this test)?

[eleftherioszisis]

Hmmm ok Is it really a feature (having morphology names of the population equal to the unresolved file name) or is it just something we check to be sure the test passed (in this case we could just change how we check this test)?

No idea. I find it strange, but maybe it was a use case in the past.
https://github.com/BlueBrain/NeuroM/tree/master/tests/data/valid_set

[adrien-berchet]

Maybe. Anyway, I guess resolving the parent only is ok, though it might still result in very surprising behavior in some very specific cases. E.g. if there is a symlink in the given path and this symlink is changed during runtime (which is very unlikely ofc), the result will be different if the symlink is in the parents or if it is the actual target.