Repository: bids-standard/pybids
Pull request #650 (2020-08-25 03:26:18)

ENH: Add CLI to PyBIDS
To streamline operations that would require a dedicated script or starting an interactive python session.

Currently just supports generation of a database index, to facilitate processing of large datasets, as well as a version check. The database index can be customized by certain kwargs for `BIDSLayout` and `BIDSLayoutIndex` classes.

This should be extendible for other operations down the line.

- Adds `click` as a dependency - its handling of sub-parsers is a bit more intuitive than `argparse` IMO.

codecov[bot]: # [Codecov](https://codecov.io/gh/bids-standard/pybids/pull/650?src=pr&el=h1) Report
> Merging [#650](https://codecov.io/gh/bids-standard/pybids/pull/650?src=pr&el=desc) into [master](https://codecov.io/gh/bids-standard/pybids/commit/39e2e9c3cb9bde1705fcd2a957f377e153bef452?el=desc) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/bids-standard/pybids/pull/650/graphs/tree.svg?width=650&height=150&src=pr&token=0d39OR1fhx)](https://codecov.io/gh/bids-standard/pybids/pull/650?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master     #650   +/-   ##
=======================================
  Coverage   84.48%   84.48%           
=======================================
  Files          28       29    +1     
  Lines        3396     3403    +7     
  Branches      844      846    +2     
=======================================
+ Hits         2869     2875    +6     
- Misses        339      340    +1     
  Partials      188      188           
```

| Flag | Coverage Δ | |
|---|---|---|
| #unittests | `84.48% <100.00%> (+<0.01%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/bids-standard/pybids/pull/650?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [bids/cli.py](https://codecov.io/gh/bids-standard/pybids/pull/650/diff?src=pr&el=tree#diff-Ymlkcy9jbGkucHk=) | `100.00% <100.00%> (ø)` | |
| [bids/analysis/model\_spec.py](https://codecov.io/gh/bids-standard/pybids/pull/650/diff?src=pr&el=tree#diff-Ymlkcy9hbmFseXNpcy9tb2RlbF9zcGVjLnB5) | `80.64% <0.00%> (-1.04%)` | :arrow_down: |
| [bids/layout/models.py](https://codecov.io/gh/bids-standard/pybids/pull/650/diff?src=pr&el=tree#diff-Ymlkcy9sYXlvdXQvbW9kZWxzLnB5) | `88.92% <0.00%> (-0.52%)` | :arrow_down: |
| [bids/layout/layout.py](https://codecov.io/gh/bids-standard/pybids/pull/650/diff?src=pr&el=tree#diff-Ymlkcy9sYXlvdXQvbGF5b3V0LnB5) | `86.53% <0.00%> (-0.15%)` | :arrow_down: |
| [bids/variables/variables.py](https://codecov.io/gh/bids-standard/pybids/pull/650/diff?src=pr&el=tree#diff-Ymlkcy92YXJpYWJsZXMvdmFyaWFibGVzLnB5) | `89.30% <0.00%> (-0.14%)` | :arrow_down: |
| [bids/layout/db.py](https://codecov.io/gh/bids-standard/pybids/pull/650/diff?src=pr&el=tree#diff-Ymlkcy9sYXlvdXQvZGIucHk=) | `98.63% <0.00%> (-0.08%)` | :arrow_down: |
| [bids/analysis/transformations/base.py](https://codecov.io/gh/bids-standard/pybids/pull/650/diff?src=pr&el=tree#diff-Ymlkcy9hbmFseXNpcy90cmFuc2Zvcm1hdGlvbnMvYmFzZS5weQ==) | `87.86% <0.00%> (-0.06%)` | :arrow_down: |
| [bids/variables/collections.py](https://codecov.io/gh/bids-standard/pybids/pull/650/diff?src=pr&el=tree#diff-Ymlkcy92YXJpYWJsZXMvY29sbGVjdGlvbnMucHk=) | `89.44% <0.00%> (-0.06%)` | :arrow_down: |
| [bids/layout/index.py](https://codecov.io/gh/bids-standard/pybids/pull/650/diff?src=pr&el=tree#diff-Ymlkcy9sYXlvdXQvaW5kZXgucHk=) | `87.75% <0.00%> (-0.05%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/bids-standard/pybids/pull/650?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/bids-standard/pybids/pull/650?src=pr&el=footer). Last update [39e2e9c...563428d](https://codecov.io/gh/bids-standard/pybids/pull/650?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

tyarkoni: This is a great idea, thanks for the PR! I'll try to review soon, but am on vacation most of this week, so maybe @effigies or @adelavega can take a look. One minor comment is that if this currently only includes functionality relevant to the `layout` module, we might want to move `cli.py` into `bids/layout`. But if you anticipate adding functionality from other modules down the line, then it's fine to leave it where it is.
mgxd: Cool, thanks! Regarding moving it to `bids.layout`, you got it - I was planning for extendibility down the line (though I'm not sure what that would be exactly...)
effigies: I feel like I would try to more closely mimic the options of `BIDSLayout`.

```
bids layout ROOT --dbpath PATH
    [--[no-]validate]
    [--derivatives [PATH [PATH ...]]]
    [--config {bids,derivatives,PATH} ...]
    # Indexer options
    [--[no-]index-metadata]
    [--ignore {PATH,/REGEX/} ...]
    [--force-index {PATH,/REGEX/} ...]
    [--config-filename PATH]
```

* `--config` can find registered configurations with `bids.config.get_option('config_paths').keys()`
* `--ignore` and `--force-index` can work without separating into paths and regexes by using `/<regex>/` pattern to identify regexes, e.g., `re.compile(arg[1:-1]) if arg[0] == arg[-1] == '/' else Path(arg)`

Does this seem reasonable?

RE other subcommands, I could see a potential for a standalone tool to generate design matrices from `bids.analysis`, so I would leave it in the top level.
effigies: @adelavega Are you thinking a subcommand of `layout`? I was thinking of that as your `save`, and then we might consider a `query` as your `get`.

So a normal process might be:

```
bids layout /path/to/dataset --dbfile dataset.db ...
bids query --dbfile dataset.db <entity>=[<value>,...]
```

But I'm not really tied to this model.
mgxd: Thanks for the reviews. A few comments:

- Sure, bringing the options to sync up with `BIDSLayout.__init__` (and subsequently `BIDSLayoutIndexer`) would be nice.
- I would prefer keeping the base CLI as `pybids <options>` to make it clear where its coming from. Also, since the whole purpose is to save the initiated layout, I feel the subcommand's name should make that clear. What about
```
pybids save-layout <options>
```
then leaving querying open down the line to be something like
```
pybids query --dbfile <path> ...
```
- I feel like `/<regex>/` is too similar to an `</absolute/path/>`, what about `r//<regex>` and then dropping the `r//`?
effigies: `save-layout` is fine, although it feels a little passive when we have to generate before we can save. What about `pybids index` or `pybids index-layout`?

I see that we do accept absolute paths in `ignore`/`force-index`, but I'm not sure why... Assuming we don't want to drop absolute paths, what about the explicit matching syntax `m/<regex>/`? That's a quite unlikely relative path pattern in BIDS. I'm just hesitant to introduce a new syntax like `r//<regex>`.
tyarkoni: If you specify a `dbpath`, it will automatically save or load as needed, so I don't know that supporting separate save/load commands is necessary. The only use case for it really is if you want to save an already-indexed layout to a different path, but that's pretty niche. So I'd probably go with @effigies's suggestion.

I like `query` better than `get`, but for consistency with the Python API, we might want to stick with `get` anyway. But I don't care much one way or the other.
adelavega: Yes, since the layout automatically saves/loads, I was thinking that just running an `index` command would create a layout and save it in one go. I also like `query` but agree `get` is more consistent with the API. 

If you know how `BIDSLayout` work you should be able to run a `get` command without previously having run an `index` command. If you don't specify a file, it would create a layout on demand, and if you specify a valid `dbpath` it would load from there, then execute the `get` command. 

The closer to the original API the better IMO.

Maybe `init` is an alternative to `index`, since all it's doing is creating a `BIDSLayout` (which happens to auto-save if given a new path).

Patch:
diff --git a/bids/cli.py b/bids/cli.py
new file mode 100644
index 00000000..8a3a543f
--- /dev/null
+++ b/bids/cli.py
@@ -0,0 +1,97 @@
+from pathlib import Path
+import click
+
+from . import __version__
+from .layout import BIDSLayoutIndexer, BIDSLayout
+from .utils import validate_multiple as _validate_multiple
+
+# alias -h to trigger help message
+CONTEXT_SETTINGS = {'help_option_names': ['-h', '--help']}
+
+
+class PathOrRegex(click.ParamType):
+    "A helper Type to parse BIDSLayoutIndexer ignore/force entries"
+    name = "path or m/regex/"
+
+    def convert(self, value, param, ctx):
+        import re
+        if re.match(r"^m/.*/$", value):  # has form "m/<regex>/"
+            value = re.compile(value[2:-1])
+        return value
+
+
+@click.group(context_settings=CONTEXT_SETTINGS)
+@click.version_option(__version__, prog_name='pybids')
+def cli():
+    """Command-line interface for PyBIDS operations"""
+    pass
+
+
+@cli.command(context_settings=CONTEXT_SETTINGS)
+@click.argument('root', type=click.Path(file_okay=False, exists=True))
+@click.argument('db-path', type=click.Path(file_okay=False, resolve_path=True, exists=True))
+@click.option('--derivatives', multiple=True, default=False, show_default=True, flag_value=True,
+              help="Specifies whether and/or which derivatives to index.")
+@click.option('--reset-db', default=False, show_default=True, is_flag=True,
+              help="Remove existing database index if present.")
+@click.option('--validate/--no-validate', default=True, show_default=True,
+              help="Check for BIDS compliance when indexing files.")
+@click.option('--config', multiple=True,
+              help="Optional name(s) of configuration file(s) to use.")
+@click.option('--index-metadata/--no-index-metadata', default=False, show_default=True,
+              help="Include metadata when indexing files.")
+@click.option('--ignore', multiple=True, type=PathOrRegex(),
+              help="Path (from root) or regex to exclude from indexing. "
+                   "Regex entries need to fitted with leading 'm/' and trailing '/'.")
+@click.option('--force-index', multiple=True, type=PathOrRegex(),
+              help="Path (from root) or regex to include when indexing. "
+                   "Regex entries need to fitted with leading 'm/' and trailing '/'.")
+@click.option('--config-filename', type=click.Path(),
+              default="layout_config.json", show_default=True,
+              help="Name of filename within directories that contains configuration information.")
+def layout(
+    root,
+    db_path,
+    derivatives,
+    reset_db,
+    validate,
+    config,
+    index_metadata,
+    ignore,
+    force_index,
+    config_filename,
+):
+    """
+    Initialize a BIDSLayout, and create an SQLite database index.
+    """
+
+    # ensure empty multiples are set to None
+    derivatives = _validate_multiple(derivatives, retval=False)
+    config = _validate_multiple(config)
+    ignore = _validate_multiple(ignore)
+    force_index = _validate_multiple(force_index)
+
+    if not (Path(db_path) / 'layout_index.sqlite').exists():
+        reset_db = True
+
+    layout = BIDSLayout(
+        root,
+        database_path=db_path,
+        reset_database=reset_db,
+        validate=validate,
+        config=config,
+        indexer=BIDSLayoutIndexer(
+            validate=validate,
+            index_metadata=index_metadata,
+            ignore=ignore,
+            force_index=force_index,
+            config_filename=config_filename,
+        ),
+    )
+    if reset_db:
+        click.echo("Successfully generated database index at {}".format(db_path))
+    else:
+        click.echo(
+            "Previously generated database index found at {}. "
+            "To generate a new index, rerun with ``--reset-db``".format(db_path)
+        )
diff --git a/bids/utils.py b/bids/utils.py
index 1676b396..100a3c19 100644
--- a/bids/utils.py
+++ b/bids/utils.py
@@ -111,3 +111,18 @@ def make_bidsfile(filename):
 
     Cls = getattr(models, cls)
     return Cls(filename)
+
+
+def validate_multiple(val, retval=None):
+    """Any click.Option with the multiple flag will return an empty tuple if not set.
+
+    This helper method converts empty tuples to a desired return value (default: None).
+    This helper method selects the first item in single-item tuples.
+    """
+    assert isinstance(val, tuple)
+
+    if val == tuple():
+        return retval
+    if len(val) == 1:
+        return val[0]
+    return val
diff --git a/setup.cfg b/setup.cfg
index 1116db34..674254d7 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -30,6 +30,7 @@ install_requires =
     sqlalchemy
     bids-validator
     num2words
+    click
 tests_require =
     pytest >=3.3
     mock
@@ -64,6 +65,10 @@ dev =
     %(doc)s
     %(test)s
 
+[options.entry_points]
+console_scripts =
+    pybids=bids.cli:cli
+
 [versioneer]
 VCS = git
 style = pep440


Test patch:
diff --git a/bids/tests/test_cli.py b/bids/tests/test_cli.py
new file mode 100644
index 00000000..ae9fec1f
--- /dev/null
+++ b/bids/tests/test_cli.py
@@ -0,0 +1,68 @@
+import os
+
+from click.testing import CliRunner
+import pytest
+
+from bids.cli import cli
+from bids.utils import validate_multiple
+from bids.tests import get_test_data_path
+
+
+@pytest.fixture
+def runner():
+    return CliRunner()
+
+
+def test_cli_entrypoint(runner):
+    res = runner.invoke(cli, catch_exceptions=False)
+    assert "Command-line interface for PyBIDS operations" in res.stdout
+    assert runner.invoke(cli, ['-h'], catch_exceptions=False).stdout == res.stdout
+    # verify versioning
+    assert runner.invoke(cli, ['--version'], catch_exceptions=False).stdout.startswith('pybids')
+
+
+def test_validate_multiple():
+    assert validate_multiple(()) is None
+    assert validate_multiple((), retval=False) is False
+    assert validate_multiple(('bids',)) == 'bids'
+    assert validate_multiple((1, 2)) == (1, 2)
+    with pytest.raises(AssertionError):
+        validate_multiple('not a tuple')
+
+
+def test_layout(runner, tmp_path):
+    def is_success(res):
+        return res.stdout.startswith("Successfully generated database index")
+
+    res = runner.invoke(cli, ['layout', '--help'])
+    assert "Initialize a BIDSLayout" in res.stdout
+
+    bids_dir = os.path.join(get_test_data_path(), 'ds005')
+    db0 = tmp_path / "db0"
+    db0.mkdir()
+    res = runner.invoke(cli, ['layout', bids_dir, str(db0)], catch_exceptions=False)
+    assert is_success(res)
+    # rerunning targeting the save directory should not generate a new index
+    res = runner.invoke(cli, ['layout', bids_dir, str(db0)], catch_exceptions=False)
+    assert not is_success(res)
+    # but forcing it should
+    res = runner.invoke(
+        cli, ['layout', bids_dir, str(db0), '--reset-db'], catch_exceptions=False
+    )
+    assert is_success(res)
+
+    db1 = tmp_path / "db1"
+    db1.mkdir()
+    # throw the kitchen sink at it
+    res = runner.invoke(
+        cli,
+        [
+            'layout', bids_dir, str(db1),
+            '--validate', '--no-index-metadata',
+            '--ignore', 'derivatives', '--ignore', 'sourcedata', '--ignore', r'm/^\./',
+            '--force-index', 'test',
+            '--config', 'bids',
+        ],
+        catch_exceptions=False,
+    )
+    assert is_success(res)