nipreps / fmriprep

fMRIPrep is a robust and easy-to-use pipeline for preprocessing of diverse fMRI data. The transparent workflow dispenses of manual intervention, thereby ensuring the reproducibility of the results.
https://fmriprep.org
Apache License 2.0
617 stars 288 forks source link

Add --omp-num-threads as alias for --omp-nthreads #2766

Open justbennet opened 2 years ago

justbennet commented 2 years ago

What would you like to see added in fMRIPrep?

It would be a courtesy to those of us who have the environment variable OMP_NUMTHREADS firmly imprinted to make --omp-numthreads an alias for --omp-nthreads. The error message I was getting when I used --omp-numthreads was about missing paths and not about an invalid option name. I think the bad option was messing up positional argument processing, so the error would depend what precedes/follows the incorrect form.

If the alias isn't viable, perhaps adding a check for that and giving a better error message would help some other poor soul in the future.

I saw that a discussion had been started on the topic of threads versus procs and memory by Chris, but no discussion had started.

I'd be interested in getting some discussion going, especially to help people who run fmriprep on clusters where they have to request cores, threads, and memory and incur some kind of charge for what they request.

Thanks for considering the request re numthreads v nthreads.

Do you have any interest in helping implement the feature?

Yes, but I would need guidance

Additional information / screenshots

No response

effigies commented 2 years ago

Could you share the error you got with --omp-numthreads? I would expect it to be an argument parsing error.

That said, I'm also fine with making --omp-numthreads an alias for --omp-nthreads.

justbennet commented 2 years ago

@effigies That was the problem -- there was no error message directly related to the OMP threads option. Here is what it says.

$ fmriprep --anat-only --fs-license-file /home/bennet/freesurfer_license.txt --nprocs 2 --omp-numthreads 1 participant -w
usage: fmriprep [-h] [--version] [--skip_bids_validation]
                [--participant-label PARTICIPANT_LABEL [PARTICIPANT_LABEL ...]]
[ help text elided ]
fmriprep: error: Path does not exist: <1>.

I think it is taking the 1 intended for --omp-numthreads as the value for bids_dir?

Any solution that results swallowing both the invalid option and its argument is fine with me. I suspect there might be others who have or will trip on prior ingrained habit. ;-)

Thanks for anything you can do.

effigies commented 2 years ago

Ah, yes there are parsing ambiguities when using both positional and flagged arguments. For this reason, I always place the positional arguments first and flags after.

justbennet commented 2 years ago

That isn't the order that is shown by fmriprep --help, though, which lists the options in the order,

$ fmriprep --help
usage: fmriprep [-h] [--version] [--skip_bids_validation]
                [--participant-label PARTICIPANT_LABEL [PARTICIPANT_LABEL ...]]
. . . .
                [--debug {compcor,fieldmaps,all} [{compcor,fieldmaps,all} ...]]
                [--sloppy]
                bids_dir output_dir {participant}

I totally agree that mixing positional and flagged leads to..., well, this situation.

I tried a few permutations, and it doesn't seem to matter where --omp-numthreads 1 appears, fmriprep still throws the fmriprep: error: Path does not exist: <1>. error. The issue is that all trailing arguments that haven't been parsed are assumed to be participants and have a directory corresponding to them, and since there can be more than one, there isn't really a way to catch them.

Would changing the error message slightly so it reads

fmriprep: error: Participant directory '1' does not exist

help? At least then people would be looking for how a 1 got interpreted as a participant.

Ideally, I guess, adding an unknown option would raise an 'unknown command option' error and stop processing, maybe?

effigies commented 2 years ago

Yes, unfortunately we have limited control over how the help text is rendered or the order in which options are processed. We're using the standard Python argparse module.

I suspect that what's happening is that the positional arguments are processed first, so the error is being raised before flags are checked.

justbennet commented 2 years ago

Something has to eat the options and their arguments first in order for --omp-numthreads to be removed but the numeral to remain, doesn't it? I thought argparse could catch illegal options. The example at https://docs.python.org/3/howto/argparse.html seems to do it. Perhaps its that there is a variable-length list of positional arguments that is complicating things.

Might this have something to help here? https://stackoverflow.com/questions/20165843/argparse-how-to-handle-variable-number-of-arguments-nargs

I suspect this situation would also arise in any simple typo in any other option name, as well, so might be worth trying to find a general recourse?

effigies commented 2 years ago

I'm happy to review a PR to improve error messaging. I don't foresee time to work on this myself, however.

oesteban commented 2 years ago

One question, isn't the variable name OMP_NUM_THREADS ? At least that's what numpy (with openblas) will expect, for instance.

justbennet commented 2 years ago

Yes, you're correct. My mistake. So, --omp_num_threads=?