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

FIX: ensure commandline aliases are respected #2152

Merged
merged 3 commits into from
Jun 1, 2020

Conversation

mgxd
Copy link
Collaborator

@mgxd mgxd commented May 29, 2020

Fixes #2151

There are many aliases for the nprocs option, but we were not being consistent with the dest variable.

Also fixes an invalid nipype plugin argument (#2153 (comment))

@auto-comment

This comment has been minimized.

@@ -150,6 +150,7 @@ def _bids_filter(value):
"--nthreads",
"--n_cpus",
"--n-cpus",
dest='nprocs',
Copy link
Member

Choose a reason for hiding this comment

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

why is not --nprocs used as dest?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i think it'd be nice to have it explicitly said, especially since this changed from 20.0.x

Copy link
Member

Choose a reason for hiding this comment

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

explicitly said

Well, developers know that argparse maps --nprocs to parsed.nprocs if it is placed first (which is implicit, but clear in their API). To the user, I don't think this makes any difference. Should we define dest for all arguments then?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if they all had multiple aliases like this one, i tend to side with "yes"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@oesteban 👍?

fmriprep/cli/parser.py Outdated Show resolved Hide resolved
@oesteban oesteban merged commit 171ac24 into nipreps:master Jun 1, 2020
@mgxd mgxd added this to the 20.1.1 milestone Jun 3, 2020
@mgxd mgxd deleted the fix/cli-parser branch June 3, 2020 21:10
mgxd added a commit that referenced this pull request Jun 4, 2020
20.1.1 (June 04, 2020)
======================
Bug-fix release in the 20.1.x series.

* FIX: FreeSurfer license manipulation & canary (#2165)
* FIX: Dismiss ``echo`` entity from SDC reports (#2160)
* FIX: Ensure the command-line alias of ``--nprocs`` is respected (#2152)
* MAINT: Use legacy pip/setuptools for py2 checking (#2156)
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.

nthreads is not properly limiting the number of jobs run in 20.1.0
3 participants