Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BART on the scanner. #8

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

schaten
Copy link

@schaten schaten commented Sep 10, 2024

Hi,
thank you for this nice project.
I had to make some changes to get the bartfire reco script running on the scanner.
Please feel free to include them in your repo.
Best
Philip

@kspaceKelvin
Copy link
Owner

Hi Philip,

Thanks for your work in improving the compatibility with BART! I had just one question -- was it necessary to increase the buffer space in the chroot image to 150 MB from 50MB for BART? Happy to make the change if needed, but if would like to preserve drive space otherwise.

@schaten
Copy link
Author

schaten commented Sep 25, 2024

Hi, thanks for looking at it!
I increased the size because it said in the instructions for generating the chroot "make sure there are at least 100MB available". Alas, I only had 70MB available after building the chroot with the script.. Apart from that, no reason. ;)
I can imagine it could lead to trouble rather easily though - because now, the memory mapped files BART works with are directly in the chroot and not on the network share anymore.
It'd be great if a "real" temporary directory is mapped into the chroot on the MARS - I don't know maybe that is even the case already.

Best
Philip

@kspaceKelvin
Copy link
Owner

Ah, in that case I would prefer that we keep the padding at 50 MB -- would you mind changing this back before we merge?

Regarding a temporary folder, when running on the MARS the /tmp/share folder is mounted by default -- You can (and should) write temporary files to this folder and they'll be written outside of the chroot image. Is there a change that we should make in BART to have it write its files there?

Thanks!
Kelvin

- mmapping on TMPDIR /tmp/share does not work
on the scanner, where this is a CIFS mounted
directory.
/tmp should be a tmpfs in the chroot on the scanner,
thus this should not fill up the chroot either.

- ENV variables aren't carried through to the chroot-image.
Thus 'import bart' wouldn't work in the chroot.
(on the github main branch we already have an installable
python module; but for now stay at v0.9.00)

- static linking leads to segfault in gfortran lib.
To resolve this for now, changed to dynamic linking and
install the runtime dependencies in the container.

- Switched bart-build container to same base image as the fire-python
container.
@@ -12,6 +12,11 @@
# Folder for debug output files
debugFolder = "/tmp/share/debug"

# Don't write to /tmp/share in cfl
os.environ["TMPDIR"] = "/tmp"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is where BART will write temporary files, maybe change this to /tmp/share instead, as matching start-fire-python-server.sh?

@schaten
Copy link
Author

schaten commented Oct 6, 2024

Hi Kelvin!
Sorry I didn't reply yet. I removed the extra padding.
However, with /tmp/share I would get an error when I tested this on the scanner, iirc.
That's why I changed that to /tmp in the first commit.
It probably has something to do with the fact that the directory is mounted from the host using CIFS (see commit message).
Unfortunately I cannot test it this week but I will try it again next week.
Best
Philip

@schaten
Copy link
Author

schaten commented Oct 17, 2024

Hi again,
so, without the fix (using /tmp) i.e. when using /tmp/share, the following happens:
The python wrapper tries to open the .cfl-file created by BART and fails with an InputOutputError.
In the logviewer it looks like this:

-1|2024/10/17-15:57:19.292685|Server.wip_070_fire_OpenRecon.Feedback|MrIrisContainer|MESSAGE_TEXT: Traceback (most recent call last):
  File "/opt/code/python-ismrmrd-server/bartfire.py", line 61, in process
    image = process_raw(acqGroup, config, metadata)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/code/python-ismrmrd-server/bartfire.py", line 153, in process_raw
    data = bart(1, 'fft -u -i 3', data)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/code/bart/python/bart.py", line 109, in bart
    output.append(cfl.readcfl(elm))
                  ^^^^^^^^^^^^^^^^
  File "/opt/code/bart/python/cfl.py", line 31, in readcfl
    with open(name + ".cfl", "rb") as d:
         ^^^^^^^^^^^^^^^^^^^^^^^^^
OSError: [Errno 5] Input/output error: '/tmp/share/tmpvsmktynlout0.cfl'¬|

If you're fast enough you can even trigger this behaviour by hand in the chroot, by creating a file with bart, then running python and try to open that.
fire-Screenshot from 2024-10-17 16-25-19

I'm not sure what's exactly the problem here to be honest. Maybe a mount option needs to be different? Or some kind of sync call is needed inbetween (however that should really not be the case...).
For me the easiest fix was just to use /tmp but maybe we find a better solution.

Best
Philip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants