-
Notifications
You must be signed in to change notification settings - Fork 2
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
[RF] Pass arbitrary arguments to FitLins, use click instead of Docopt #150
Conversation
neuroscout_cli/commands/run.py
Outdated
] | ||
|
||
# Append pass through options | ||
fitlins_args.append(list(self.options['fitlings_args'])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does fitlins handle duplicate arguments? The last one seen in the list is valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does there need to be an analysis level in the fitlins args?
defaults to dataset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fitlins uses argparse, it currently uses the last seen value for an argument if the same argument is specified multiple time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except where nargs='+', like ignore, where all instances will be collected into a list. So overwriting that option completely is not possible as fitlins args are currently built/called here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall seems good to me.
May be useful to dump fitlins_args alongside original options to exactly what is passed to fitlins or else document options implicitly set by the cli.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like click a lot better, this looks nice! I just have a couple questions about how fitlins_args
should be handled and how the install command works.
Could you add an example on how a user would pass in arbitrary fitlins arguments?
neuroscout_cli/commands/run.py
Outdated
] | ||
|
||
# Append pass through options | ||
fitlins_args.append(list(self.options['fitlings_args'])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the --model
, --ignore
, and --derivatives
arguments be removed from fitlins_args (if present) or would you want users to be able to overwrite those arguments?
minor typo
fitlins_args.append(list(self.options['fitlings_args'])) | |
fitlins_args.append(list(self.options['fitlins_args'])) |
|
||
Commands: | ||
run Runs analysis. | ||
install Installs a bundle and/or dataset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coming at this naively, it feels a little weird that when I install
I could specify two paths, one for the dataset and one for the bundle, i don't know if this should be split into two different commands so one could install the dataset (only) or install the bundle (only). looks like the current options are:
- install bundle
- install bundle + data
For me I was expecting a third option:
- install bundle
- install data (new)
- install both
That helped conceptually clear up that the install command can install two separate things in two separate directories.
but perhaps this is because I did not read the documentation closely enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jdkent I think the real answer is you need the analysis bundle to download the data. So currently it's not possible to only download the data. that said, if the bundle is already downloaded, then you could get the data only, so I think i'll add flags for this on the new get
command.
also add : and document default fitlins arguments (i.e. |
This is a major refactor to the CLI, particularly with respect to the argument parsing.
docopt
and useclick
insteadUpload
andRun/Install
Usage docs are separated based on the sub-command, of which there are three:
run
,install
andupload
The overall usage is:
The main command
run
is used as follows:Importantly, any arguments passed as
[FITLINS_ARGS]
will be simply passed through to FitLins.The other commands
upload
andinstall
are ways to do the analysis pieacemeal rather than havingrun
do everything for you:e.g.:
and:
Any feedback on this usage pattern? (cc: @effigies) Still need to test it more, and will update the documentation on the main Neuroscout docs for this.