Deep-MI / FastSurfer

PyTorch implementation of FastSurferCNN
Apache License 2.0
435 stars 115 forks source link

recon_surf//functions.sh: line 38: -debug: command not found #528

Closed constructor-s closed 1 week ago

constructor-s commented 2 weeks ago

It seems fs_time ($timecmd) should support a -debug flag but when called in the following manner on a native installation on my Mac (M1, zsh 5.9 (x86_64-apple-darwin22.0), GNU bash, version 5.2.26(1)-release (aarch64-apple-darwin22.6.0)), it does not like it. When I removed it it seemed to proceed okay. Not sure if it is an issue with the Mac shell.

https://github.com/Deep-MI/FastSurfer/blob/3622c13c049a367e72ca9c0db8332c0493101db7/recon_surf/functions.sh#L38

...

./recon-surf.sh --sid 140 --sd /Users/USERNAME/my_fastsurfer_analysis --t1 /Users/USERNAME/my_fastsurfer_analysis/140/mri/orig.mgz --asegdkt_segfile /Users/USERNAME/my_fastsurfer_analysis/140/mri/aparc.DKTatlas+aseg.deep.mgz --threads 1 --parallel

freesurfer-macOS-darwin_x86_64-7.4.1-20230614-7eb8460
$Id$
Darwin USERNAMEs-MacBook-Pro-2.local 22.6.0 Darwin Kernel Version 22.6.0: Mon Apr 22 20:50:39 PDT 2024; root:xnu-8796.141.3.705.2~1/RELEASE_ARM64_T8103 arm64

================== Checking validity of inputs =================================

 RUNNING both hemis in PARALLEL 
 RUNNING 1 number of OMP THREADS 
 RUNNING 1 number of ITK THREADS 

Checking Input Segmentation Quality ...
python3.10 /Users/USERNAME/FastSurfer_dev/FastSurferCNN/quick_qc.py --asegdkt_segfile /Users/USERNAME/my_fastsurfer_analysis/140/mri/aparc.DKTatlas+aseg.deep.mgz
/Users/USERNAME/FastSurfer_dev/recon_surf//functions.sh: line 38: -debug: command not found
m-reuter commented 2 weeks ago

Hi, not sure what is going on maybe @dkuegler can take a look as I found this commit: https://github.com/Deep-MI/FastSurfer/commit/56ac5fb998f0d05150f1905c5e5e5393d948c225 which says it adds support for the -debug flag.

dkuegler commented 1 week ago

First off, thank you for reporting this issue.

This should really have been caught before. It never raised an issue for me, because I usually test on Ubuntu and our mac testing is not as robust as it needs to be :(.

In particular, the -debug does not need to be in there. But it specifically only leads to a real problem, if timecmd is set to empty (see https://github.com/Deep-MI/FastSurfer/blob/3622c13c049a367e72ca9c0db8332c0493101db7/recon_surf/functions.sh#L15).

The fix is equally as simple, the -debug was a remnant of debugging in any case.

m-reuter commented 1 week ago

Has been merged into dev yesterday. Thanks

constructor-s commented 1 week ago

Thanks!

Speaking of Mac testing, the other change I had to make was to change all #!/bin/bash to #!/usr/bin/env bash since, at least if the updated version of bash is installed through homebrew, the former is always GNU bash, version 3.2.57(1)-release (arm64-apple-darwin22) while the later dynamically goes to GNU bash, version 5.2.26(1)-release (aarch64-apple-darwin22.6.0). Even if the initial entry script was called with bash ... which uses the correct version 5 bash, some of the .sh scripts were calling other scripts and those were executed by version 3 bash which some scripts don't like. Happy to make another issue if you'd like.

m-reuter commented 1 week ago

Thanks, yes, I had noticed that also. There is two ways to fix it:

  1. either - as you propose - modify the first line of the scripts (and we need to check that this is portable across linux and windows versions!), or

  2. replace all |&with 2>&1 |which should be supported in older bash versions. I think this option makes more sense (and I hope that that is the only feature from bash v4.0 that we use).

m-reuter commented 1 week ago

I implemented option 2, so only bash 3.2 is now required. Also updated the requirements.mac.txt to more modern versions (and excluding some versions that break things, but maybe not all).