Fix relative path errors when saving Trajectories

[PardhavMaradani]

This PR fixes errors that are thrown due to incorrect relative paths when saving Trajectory entities.

Here is a simple script which when run from a Notebook shows the problem:

import warnings
import bpy
import MDAnalysis as mda
from MDAnalysis.tests.datafiles import DCD, PSF
import molecularnodes as mn

warnings.filterwarnings("ignore")
u = mda.Universe(PSF, DCD)
canvas = mn.Canvas()

t = mn.entities.Trajectory(u).add_style("cartoon")
bpy.ops.wm.save_as_mainfile(filepath="/tmp/test.blend")

Show Traceback

Error in bpy.app.handlers.save_post[0]:
---------------------------------------------------------------------------
OSError                                   Traceback (most recent call last)
File ~/blender/blender-5.0.0-linux-x64/5.0/python/lib/python3.11/site-packages/molecularnodes/session.py:320, in _pickle(filepath)
    318 @persistent
    319 def _pickle(filepath) -> None:
--> 320     get_session().pickle(filepath)

File ~/blender/blender-5.0.0-linux-x64/5.0/python/lib/python3.11/site-packages/molecularnodes/session.py:160, in MNSession.pickle(self, filepath)
    157 def pickle(self, filepath) -> None:
    158     pickle_path = self.stashpath(filepath)
--> 160     make_paths_relative(self.trajectories)
    161     self.entities = trim(self.entities)
    163     # don't save anything if there is nothing to save

File ~/blender/blender-5.0.0-linux-x64/5.0/python/lib/python3.11/site-packages/molecularnodes/session.py:30, in make_paths_relative(trajectories)
     27 for key, traj in trajectories.items():
     28     # save linked universe frame
     29     uframe = traj.uframe
---> 30     traj.universe.load_new(make_path_relative(traj.universe.trajectory.filename))
     31     # restore linked universe frame
     32     traj.uframe = uframe

File ~/blender/blender-5.0.0-linux-x64/5.0/python/lib/python3.11/site-packages/MDAnalysis/core/universe.py:876, in Universe.load_new(self, filename, format, in_memory, in_memory_step, **kwargs)
    873 # supply number of atoms for readers that cannot do it for themselves
    874 kwargs["n_atoms"] = self.atoms.n_atoms
--> 876 self.trajectory = reader(filename, format=format, **kwargs)
    877 if self.trajectory.n_atoms != len(self.atoms):
    878     raise ValueError(
    879         "The topology and {form} trajectory files don't"
    880         " have the same number of atoms!\n"
   (...)    887         )
    888     )

File ~/blender/blender-5.0.0-linux-x64/5.0/python/lib/python3.11/site-packages/MDAnalysis/lib/util.py:2701, in store_init_arguments.<locals>.wrapper(self, *args, **kwargs)
   2699             else:
   2700                 self._kwargs[key] = arg
-> 2701 return func(self, *args, **kwargs)

File ~/blender/blender-5.0.0-linux-x64/5.0/python/lib/python3.11/site-packages/MDAnalysis/coordinates/DCD.py:150, in DCDReader.__init__(self, filename, convert_units, dt, **kwargs)
    132 """
    133 Parameters
    134 ----------
   (...)    146    Changed to use libdcd.pyx library and removed the correl function
    147 """
    148 super(DCDReader, self).__init__(
    149     filename, convert_units=convert_units, **kwargs)
--> 150 self._file = DCDFile(self.filename)
    151 self.n_atoms = self._file.header['natoms']
    153 delta = mdaunits.convert(self._file.header['delta'],
    154                          self.units['time'], 'ps')

File ~/blender/blender-5.0.0-linux-x64/5.0/python/lib/python3.11/site-packages/MDAnalysis/lib/formats/libdcd.pyx:168, in MDAnalysis.lib.formats.libdcd.DCDFile.__cinit__()

File ~/blender/blender-5.0.0-linux-x64/5.0/python/lib/python3.11/site-packages/MDAnalysis/lib/formats/libdcd.pyx:257, in MDAnalysis.lib.formats.libdcd.DCDFile.open()

OSError: DCD file does not exist

The problem is not specific to API use, but can also be reproduced with regular Blender GUI. The issue arises because Blender Python's current working directory can be very different based on how it is launched (say when launching through jupyter, command line or from GUI). The relative paths made during picking are not necessarily valid and hence leads to errors both during pickling and later during unpickling as those files will not be found.

This PR uses a context manager to temporarily change the current directory to that of the blend file before pickling and unpickling, which fixes the problem. Thanks

[PardhavMaradani]

Hi @BradyAJohnston , I didn't quite understand what's happening in make_path_relative. (Just returning the filepath after the relpath conversion works) The above test failures are also due to it. I tried this standalone in a notebook and saw weird results:

import os

def trim_root_folder(filename):
    "Remove one of the prefix folders from a filepath"
    return os.sep.join(filename.split(os.sep)[1:])

def make_path_relative(filepath):
    "Take a path and make it relative, in an actually usable way"
    try:
        filepath = os.path.relpath(filepath)
    except ValueError:
        return filepath

    # count the number of "../../../" there are to remove
    n_to_remove = int(filepath.count("..") - 2)
    # get the filepath without the huge number of "../../../../" at the start
    sans_relative = filepath.split("..")[-1]

    if n_to_remove < 1:
        return filepath

    for i in range(n_to_remove):
        sans_relative = trim_root_folder(sans_relative)

    return f"./{sans_relative}"

make_path_relative("/tmp/adk_dims.dcd")

prints:

'./'

make_path_relative("/tmp/x/y/adk_dims.dcd")

prints:

'./adk_dims.dcd'

I can close this and raise it as an issue if you want to take a look. I hope you are able to reproduce the original issue - I just had adk.psf and adk_dims.dcd in /tmp that I am loading from the GUI of a Blender that is launched from a notebook and saving it to a file in /tmp that threw the original error. Thanks

[PardhavMaradani]

Hi Brady, do we really need to convert the md trajectory filename to a relative path? After a bit more testing, it seems to me that this creates more problems than it solves. Even basic portability on a single machine is broken due to this. This PR converts the trajectory filename relative to the saved blend (and MNSession) file which works for the save and load scenarios, but is still not good enough.

Let me explain. MDAnalysis supports a list of files as well for the coordinates, so this doesn't work for those cases (when used from the API, since GUI only supports one trajectory file currently). Blender python's cwd is completely dependent on how it is launched (even when launching the same instance of the binary), so the question of "relative to what?" becomes important. Even changing this relative to the saved blend file (in the saved cases) means that the cwd has to be set to that for things to work correctly - this can be controlled for save and load (like in this PR), but MDAnalysis reloads the trajectory files during some analysis runs, esp when it has to rewind the trajectory - all of those cases fill fail to find the file. (A simple test for this is to initialize dssp after the file is saved)

I would recommend removing this conversion all together. If we need to support portability across machines etc, we could potentially expose these filenames - maybe in the Trajectory panel in the n-panel for these to be configurable at runtime - updates of which will call the load_new of the universe. This seems to be the safer choice. Thanks

[BradyAJohnston]

It's been a while since I revisited the filepath stuff but it has been on my list of "janky things to fix" for a while.

The original idea was to save the filepaths relative to the .blend file, which solved an issue where a user would move files onto a remote server for rendering and would need the it to reload (and be relative not absolute) to do so.

I agree the current setup is a bit of a mess and does currently fail in some situations. I think we need to make it clear what the path ahead is that needs to be taken though. I don't have time to look over code for a few days but I should be able to get to this soon.

The main points / issues that I know around feilpaths are this:

• If a user loads files without first saving a .blend file - it's impossible to save a filepath relative to the .blend file because it doesn't have a location on disk.
  • The current idea is that it stays absolute until a .blend is saved, at which point we attempt to make the filepaths relative to the current .blend rather than any working directory.
  • Blender makes filepaths relative with the // prefix - but no other program works with that format so after getting a relative path from the .blend we need to convert it back to absolute before actually opening anything but I can't quite remember. This is definitely why it ended up being so messy.
• We need to be able to support moving a folder that contains a .blend and needed files (trajectory / topology) to another machine and it should be able to "just load" if opened there.

[PardhavMaradani]

If the behaviour needs to be as described in the previous comment, we should probably just set the cwd to the blend file directory when saving and loading. This is not ideal because other addons can also change the cwd at will. But this will at least fix the save and load issues and also ensure that any MDAnalysis reload of the trajectory also works. I have updated the PR accordingly. Thanks

[BradyAJohnston]

We definitely need to avoid setting the cwd - we can't rely on it and is always a recipe for potential problems.

[PardhavMaradani]

Hi Brady, if we want to change the trajectory path to relative (to the .blend file) and have it work with both picking/unpickling and MDAnalysis when it has to reload the trajectory (for any number of reasons), I don't think there is any other option than to change the cwd. We unfortunately, cannot have it both ways. The other alternative is to just rely on abs paths and not do this conversion.

The very first commit sets the cwd temporarily during pickling/unpickling (and restores it back), which works for saving and loading the session, but when MDAnalysis tries to load the trajectory later (say for example when you initialize DSSP) that won't work as the file is relative to somewhere else. We cannot control both the contexts. Hope I've explained the problem correctly. Thanks

[PardhavMaradani]

Hi Brady, the latest commit has something workable that fixes all the current issues with save/load and MDAnalysis trajectory reader. There are larger issues with pickling/unpickling that we'll have to address in the future - I'll try raise them as a separate issue.

The current changes do the following:
During pickling, the current directory is temporarily set to that of the blend file, the trajectory paths changed relative to the blend file, the trajectory re-loaded (using load_new) so that the instance references the relative path in the data that gets pickled. After the pickled session is saved, the trajectory paths are reverted back to the absolute paths and the trajectory re-loaded. Without this, regular trajectory reader operations will fail to load the file correctly after the save operation.

During unpickling, the current directory is again temporarily set to that of the blend file, the trajectory paths set back to absolute paths after unpickling and re-loaded.

There is no permanent change of cwd involved - only a temporary contextlib.chdir during pickling/unpickling the session. The trajectory entity will also continue to work as is after a save, save and reload, save and reload from a new base dir, etc. Thanks