MRtrix3 / mrtrix3

MRtrix3 provides a set of tools to perform various advanced diffusion MRI analyses, including constrained spherical deconvolution (CSD), probabilistic tractography, track-density imaging, and apparent fibre density
http://www.mrtrix.org
Mozilla Public License 2.0
294 stars 181 forks source link

Python image piping not working on MSYS2 #2960

Open Lestropie opened 3 months ago

Lestropie commented 3 months ago

Issue with #2678.

When constructing the location of a piped image, on Windows the current working directory is used (assuming TmpFileDir is not set), given that /tmp/ does not exist. This currently yields a Posix-style path.

However if the output of a Python script is piped to a C++ binary, the latter fails to locate that file, and the piped image remains on the file system. The C++ binary can be subsequently run standalone, copy-pasting that absolute path as input, and that works (presumably because the terminal emulator / shell is doing a path translation that is absent when that path is communicated via a pipe). Curiously, piping from a Python command to another Python command works.

If to resolve completely is going to require translation of paths to Windows format prior to sending down the pipe, I found this example, which invokes cygwin functionality via shared DLL; otherwise it could be custom code. Also would need to fill app._STDOUT_IMAGES with instances of a custom class that uses the standard Posix path f-string formatting when invoking a command to generate it but a different function to write the path to stdout at script completion.

Open to any better ideas.

Lestropie commented 3 months ago

It can be remedied temporarily by setting TmpFileDir to a Windows-style path in an MRtrix config file. Use of the -config command-line option however does not work, as piped image paths are determined at command-line parsing time whereas the config option only has an effect after command-line parsing has completed (I can potentially fix this by deferring the determination of a filesystem path until the first time it is used).

daljit46 commented 3 months ago

When constructing the location of a piped image, on Windows the current working directory is used (assuming TmpFileDir is not set), given that /tmp/ does not exist. This currently yields a Posix-style path.

Under an MSYS2 shell, /tmp/ should exist no?

Lestropie commented 3 months ago

Hmmm so it does. I think I've gotten a couple a couple of wires crossed in my reasoning / description. There's been logic in place for quite some time to use a different default location for temporary piped files between Unix and Windows; see 9979917dbaf77c045ea9115c62a388de02585494 & #199, though I don't think that's exhaustive regarding prior discussion on the topic. I seem to recall there was stronger reasoning at the time about preserving use of the working directory on Windows.

Nevertheless that observation doesn't provides a solution to the issue at hand: passing a /tmp/mrtrix-tmp-*.mif piped image through a pipe doesn't work either, it's still a Posix path that can't be interpreted by the downstream C++ binary receiving it.

The image path passed down the pipe has to be absolute, as the invoked command that generates it and the command at the receiving end of the pipe have different working directories. So either:

While we could hypothetically not support this feature on Windows, and immediately report an error if the user attempts to create an output piped image from a Python command, there are Python scripts that now internally make use of Python command piping, so the source code for those commands would need to be reverted accordingly.

daljit46 commented 3 months ago

The C++ API needs to translate input piped image paths from Posix to Windows.

My expectation is that using std::filesystem::path to handle image paths should be enough to deal with this. I'm still working on a PR to deal with #2585.

Lestropie commented 3 months ago

I had a little bit of a look at std::filesystem::path, and didn't find anything that looked like it was going to do that conversion simply. Given that hypothetically one could send the piped output from an MRtrix3 command to a non-MRtrix3 command, I tend to think that whatever's emitted to stdout should be natively interpretable, in which case mrtrix3.app._execute() should be doing the translation. I might give that a go.

jdtournier commented 3 months ago

Feel free to ignore if I'm completely wide of the mark, but I had to grapple with MSYS2's internal path translation while sorting out a few issues with the build and configure scripts some time ago, which may be useful in this context. Take a look at #1176, and the use of cygpath to perform the translation. No idea whether it'll be any use here, but just in case...

Lestropie commented 3 months ago

I've already landed on using subprocess to call cygpath 😀