Repository: bids-standard/pybids

Issue:
Collect database files into a single zipped file
Writing database files for derivatives to the respective database directories is not ideal because it adds a non-bids file to the directory and because the layout database file may not be the entire derivatives directory based on ignore and force_index options. As a solution we should instead use an archive containing database files for the main bids directory and all derivatives directories.

Discussion:

Patch:
diff --git a/bids/layout/layout.py b/bids/layout/layout.py
index 76afc01e..9b5d84df 100644
--- a/bids/layout/layout.py
+++ b/bids/layout/layout.py
@@ -10,6 +10,7 @@ import copy
 import warnings
 import sqlite3
 import enum
+from pathlib import Path
 
 import sqlalchemy as sa
 from sqlalchemy.orm import joinedload
@@ -171,18 +172,18 @@ class BIDSLayout(object):
         search (False, default) when comparing the query string to each
         entity in .get() calls. This sets a default for the instance, but
         can be overridden in individual .get() requests.
-    database_file : str
-        Optional path to SQLite database containing the
-        index for this BIDS dataset. If a value is passed and the file
+    database_path : str
+        Optional path to directory containing SQLite database file index
+        for this BIDS dataset. If a value is passed and the folder
         already exists, indexing is skipped. By default (i.e., if None),
         an in-memory SQLite database is used, and the index will not
         persist unless .save() is explicitly called.
     reset_database : bool
-        If True, any existing file specified in the
-        database_file argument is deleted, and the BIDS dataset provided
+        If True, any existing directory specified in the
+        database_path argument is deleted, and the BIDS dataset provided
         in the root argument is reindexed. If False, indexing will be
         skipped and the existing database file will be used. Ignored if
-        database_file is not provided.
+        database_path is not provided.
     index_metadata : bool
         If True, all metadata files are indexed at
         initialization. If False, metadata will not be available (but
@@ -195,8 +196,8 @@ class BIDSLayout(object):
     def __init__(self, root, validate=True, absolute_paths=True,
                  derivatives=False, config=None, sources=None, ignore=None,
                  force_index=None, config_filename='layout_config.json',
-                 regex_search=False, database_file=None, reset_database=False,
-                 index_metadata=True):
+                 regex_search=False, database_path=None, database_file=None,
+                 reset_database=False, index_metadata=True):
         """Initialize BIDSLayout."""
         self.root = root
         self.validate = validate
@@ -206,16 +207,15 @@ class BIDSLayout(object):
         self.regex_search = regex_search
         self.config_filename = config_filename
 
-        def _normalize_db_name(default_root_dir, db_file):
-            if db_file is None:
-                return None
-            # If a full absolute path is given, use it, else make path
-            if not os.path.isabs(db_file):
-                db_file = os.path.abspath(
-                    os.path.join(default_root_dir, db_file))
-            return db_file
-
-        database_file = _normalize_db_name(self.root, database_file)
+        if database_file is not None:
+            database_path = database_file
+            warnings.warn(
+                'In pybids 0.10 database_file argument was deprecated in favor '
+                'of database_path, and will and will be removed in 0.12. '
+                'For now, treating database_file as a directory.',
+                DeprecationWarning)
+        if database_path:
+            database_path = str(Path(database_path).absolute())
 
         self.session = None
 
@@ -236,7 +236,7 @@ class BIDSLayout(object):
         # Initialize the BIDS validator and examine ignore/force_index args
         self._validate_force_index()
 
-        index_dataset = self._init_db(database_file, reset_database)
+        index_dataset = self._init_db(database_path, reset_database)
 
         if index_dataset:
             # Create Config objects
@@ -263,15 +263,14 @@ class BIDSLayout(object):
         if derivatives:
             if derivatives is True:
                 derivatives = os.path.join(root, 'derivatives')
-            # if creating db files for bids, also create db files for derivs
-            create_derivative_database_files = (database_file is not None)
             self.add_derivatives(
-                derivatives, create_derivative_database_files=create_derivative_database_files,
+                derivatives, parent_database_path=database_path,
                 validate=validate, absolute_paths=absolute_paths,
-                derivatives=None, config=None, sources=self, ignore=ignore,
+                derivatives=None, sources=self, ignore=ignore,  config=None,
                 force_index=force_index, config_filename=config_filename,
-                regex_search=regex_search, reset_database=index_dataset or reset_database,
-                index_metadata=index_metadata)
+                regex_search=regex_search, index_metadata=index_metadata,
+                reset_database=index_dataset or reset_database
+                )
 
     def __getattr__(self, key):
         """Dynamically inspect missing methods for get_<entity>() calls
@@ -354,22 +353,57 @@ class BIDSLayout(object):
 
         self.session = sa.orm.sessionmaker(bind=engine)()
 
-    def _init_db(self, database_file=None, reset_database=False):
+    def _make_db_paths(self, database_path):
+        if database_path is not None:
+            database_file = os.path.join(database_path, 'layout_index.sqlilte')
+            database_sidecar = os.path.join(database_path, 'layout_args.json')
+            os.makedirs(database_path, exist_ok=True)
+        else:
+            database_file = None
+            database_sidecar = None
+        return database_file, database_sidecar
+
+    def _sanitize_instance_args(self):
+        instance_args = {
+            a: self.__dict__[a] for a in
+            ['root', 'validate', 'absolute_paths', 'regex_search', 'config_file']
+            if a in self.__dict__
+        }
+        for k in ['ignore', 'force_index']:
+            instance_args[k] = [str(a) for a in self.__dict__[k]]
+        return instance_args
+
+    def _init_db(self, database_path=None, reset_database=False):
+        database_file, database_sidecar = self._make_db_paths(database_path)
         # Reset database if needed and return whether or not it was reset
         # determining if the database needs resetting must be done prior
-        # prior to setting the session (which creates the empty database file)
+        # to setting the session (which creates the empty database file)
         reset_database = (
             reset_database or  # Manual Request
-            not database_file or  # In memory transient database
+            not database_path or  # In memory transient db
             not os.path.exists(database_file)  # New file based db created
         )
 
         self._set_session(database_file)
 
-        if reset_database:
+        instance_args = self._sanitize_instance_args()
+
+        if not reset_database:
+            with open(database_sidecar) as fobj:
+                saved_args = json.load(fobj)
+            for k, v in saved_args.items():
+                if instance_args[k] != v:
+                    raise ValueError(
+                        "Initialization arguments do not match for database_path:"
+                        " {}".format(database_path)
+                        )
+        else:
             engine = self.session.get_bind()
             Base.metadata.drop_all(engine)
             Base.metadata.create_all(engine)
+            if database_sidecar:
+                json.dump(instance_args, open(database_sidecar, 'w'))
+
             return True
 
         return False
@@ -487,14 +521,19 @@ class BIDSLayout(object):
         """Get the files."""
         return self.get_files()
 
-    def save(self, filename='.index.db', replace_connection=True):
+    def save(self, database_path, replace_connection=True):
         """Save the current index as a SQLite3 DB at the specified location.
 
+        Note: This is only necessary if a database_path was not specified
+        at initalization, and the user now wants to save the index.
+        If a database_path was specified originally, there is no need to re-save
+        using this method.
+
         Parameters
         ----------
-        filename : str, optional
-            The path to the desired database file. By default,
-            uses .index.db. If a relative path is passed, it is assumed to
+        database_path : str
+            The path to the desired database folder. By default,
+            uses .db_cache. If a relative path is passed, it is assumed to
             be relative to the BIDSLayout root directory.
         replace_connection : bool, optional
             If True, the newly created database will
@@ -505,8 +544,8 @@ class BIDSLayout(object):
             be reflected in the new file unless save() is explicitly called
             again.
         """
-        filename = os.path.join(self.root, filename)
-        new_db = sqlite3.connect(filename)
+        database_file, database_sidecar = self._make_db_paths(database_path)
+        new_db = sqlite3.connect(database_file)
         old_db = self.session.get_bind().connect().connection
 
         with new_db:
@@ -516,7 +555,16 @@ class BIDSLayout(object):
             new_db.commit()
 
         if replace_connection:
-            self._set_session(filename)
+            self._set_session(database_file)
+
+        # Dump instance arguments to JSON
+        instance_args = self._sanitize_instance_args()
+        json.dump(instance_args, open(database_sidecar, 'w'))
+
+        # Recursively save children
+        for pipeline_name, der in self.derivatives.items():
+            der.save(os.path.join(
+                database_path, pipeline_name))
 
     def get_entities(self, scope='all', metadata=None):
         """Get entities for all layouts in the specified scope.
@@ -620,8 +668,7 @@ class BIDSLayout(object):
         return parse_file_entities(filename, entities, config,
                                    include_unmatched)
 
-    def add_derivatives(self, path, create_derivative_database_files=False,
-                        **kwargs):
+    def add_derivatives(self, path, parent_database_path=None, **kwargs):
         """Add BIDS-Derivatives datasets to tracking.
 
         Parameters
@@ -631,10 +678,10 @@ class BIDSLayout(object):
             Each path can point to either a derivatives/ directory
             containing one more more pipeline directories, or to a single
             pipeline directory (e.g., derivatives/fmriprep).
-        create_derivative_database_files : bool
-            If True, use the pipline name from the dataset_description.json
-            file as the database filename and write the file to disk in the
-            derivatives directory.
+        parent_database_path : str
+            If not None, use the pipeline name from the dataset_description.json
+            file as the database folder name to nest within the parent database
+            folder name to write out derivatie index to.
         kwargs : dict
             Optional keyword arguments to pass on to
             BIDSLayout() when initializing each of the derivative datasets.
@@ -692,9 +739,10 @@ class BIDSLayout(object):
             # Default config and sources values
             kwargs['config'] = kwargs.get('config') or ['bids', 'derivatives']
             kwargs['sources'] = kwargs.get('sources') or self
-            if create_derivative_database_files:
-                current_database_file = os.path.join(deriv, pipeline_name + ".sqlite")
-                kwargs['database_file'] = current_database_file
+            if parent_database_path:
+                child_database_path = os.path.join(
+                    parent_database_path, pipeline_name)
+                kwargs['database_path'] = child_database_path
 
             self.derivatives[pipeline_name] = BIDSLayout(deriv, **kwargs)
 
@@ -787,7 +835,7 @@ class BIDSLayout(object):
             Keys are entity names, values are regexes to filter on. For
             example, passing filters={'subject': 'sub-[12]'} would return
             only files that match the first two subjects. In addition to
-            ordinary data types, the following enums are defined (in the 
+            ordinary data types, the following enums are defined (in the
             Query class):
                 * Query.NONE: The named entity must not be defined.
                 * Query.ANY: the named entity must be defined, but can have any
@@ -807,7 +855,7 @@ class BIDSLayout(object):
             filters['extension'] = filters.pop('extensions')
             warnings.warn("In pybids 0.9.0, the 'extensions' filter was "
                           "deprecated in favor of 'extension'. The former will"
-                          " stop working in 0.11.0.")
+                          " stop working in 0.11.0.", DeprecationWarning)
 
         layouts = self._get_layouts_in_scope(scope)
 


Test patch:
diff --git a/bids/layout/tests/conftest.py b/bids/layout/tests/conftest.py
index 1eb2cae9..88e91c8f 100644
--- a/bids/layout/tests/conftest.py
+++ b/bids/layout/tests/conftest.py
@@ -1,7 +1,7 @@
 from os.path import join
 
 import pytest
-
+import tempfile
 from bids.layout import BIDSLayout
 from bids.tests import get_test_data_path
 
@@ -40,28 +40,30 @@ def layout_ds005_derivs():
     return layout
 
 
-@pytest.fixture(scope="module")
-def layout_ds005_multi_derivs():
+@pytest.fixture(scope="session")
+def db_dir(tmpdir_factory):
+    fn = tmpdir_factory.mktemp("data")
+    return fn
+
+
+@pytest.fixture(scope="module",
+                params=[None, "bidsdb", "bidsdb"])
+def layout_ds005_multi_derivs(request, db_dir):
     data_dir = join(get_test_data_path(), 'ds005')
-    layout = BIDSLayout(data_dir)
+    database_path = str(db_dir / request.param) if request.param else None
+
+    layout = BIDSLayout(data_dir,
+                        database_path=database_path)
     deriv_dir1 = join(get_test_data_path(), 'ds005_derivs')
     deriv_dir2 = join(data_dir, 'derivatives', 'events')
     layout.add_derivatives([deriv_dir1, deriv_dir2])
     return layout
 
 
-@pytest.fixture(scope="module")
-def layout_synthetic():
-    path = join(get_test_data_path(), 'synthetic')
-    return BIDSLayout(path, derivatives=True)
-
-
-@pytest.fixture(scope="module")
-def layout_synthetic_cached_db():
-    path = join(get_test_data_path(), 'synthetic')
-    return BIDSLayout(path, derivatives=True, database_file="bidsdb.sqlite")
-
-@pytest.fixture(scope="module")
-def layout_synthetic_cached_db_replay():
+@pytest.fixture(
+    scope="module", params=[None, "bidsdb-synth", "bidsdb-synth"])
+def layout_synthetic(request, db_dir):
     path = join(get_test_data_path(), 'synthetic')
-    return BIDSLayout(path, derivatives=True, database_file="bidsdb.sqlite")
+    database_path = str(db_dir / request.param) if request.param else None
+    return BIDSLayout(path, derivatives=True,
+                      database_path=database_path)
diff --git a/bids/layout/tests/test_layout.py b/bids/layout/tests/test_layout.py
index a0197680..b5f9ffe8 100644
--- a/bids/layout/tests/test_layout.py
+++ b/bids/layout/tests/test_layout.py
@@ -12,13 +12,11 @@ import pytest
 import bids
 from bids.layout import (BIDSLayout, parse_file_entities, add_config_paths,
                          Query)
-from bids.layout.models import (BIDSFile, BIDSImageFile, Entity, Config,
-                                FileAssociation)
+from bids.layout.models import Entity, Config
 from bids.tests import get_test_data_path
 from bids.utils import natural_sort
 
 
-
 def test_layout_init(layout_7t_trt):
     assert isinstance(layout_7t_trt.files, dict)
 
@@ -60,7 +58,7 @@ def test_get_file(layout_ds005_derivs):
     assert not layout.get_file(target, scope='derivatives')
 
     # absolute path in BIDS-Raw
-    target = (layout.root + '/' +  orig_file).split('/')
+    target = (layout.root + '/' + orig_file).split('/')
     target = os.path.sep + os.path.join(*target)
     assert layout.get_file(target)
     assert layout.get_file(target, scope='raw')
@@ -74,7 +72,7 @@ def test_get_file(layout_ds005_derivs):
     assert layout.get_file(target, scope='derivatives')
 
     # absolute path in derivatives pipeline
-    target = (layout.root + '/' +  orig_file).split('/')
+    target = (layout.root + '/' + orig_file).split('/')
     target = os.path.sep + os.path.join(*target)
     assert layout.get_file(target)
     assert not layout.get_file(target, scope='raw')
@@ -136,8 +134,8 @@ def test_get_metadata_meg(layout_ds117):
 def test_get_metadata5(layout_7t_trt):
     target = 'sub-01/ses-1/func/sub-01_ses-1_task-rest_acq-fullbrain_run-1_bold.nii.gz'
     target = target.split('/')
-    result = layout_7t_trt.get_metadata(join(layout_7t_trt.root, *target),
-                                      include_entities=True)
+    result = layout_7t_trt.get_metadata(
+        join(layout_7t_trt.root, *target), include_entities=True)
     assert result['EchoTime'] == 0.020
     assert result['subject'] == '01'
     assert result['acquisition'] == 'fullbrain'
@@ -232,14 +230,17 @@ def test_get_val_none(layout_7t_trt, acq):
     t1w_files = layout_7t_trt.get(subject='01', ses='1', suffix='T1w')
     assert len(t1w_files) == 1
     assert 'acq' not in t1w_files[0].path
-    t1w_files = layout_7t_trt.get(subject='01', ses='1', suffix='T1w', acquisition=acq)
+    t1w_files = layout_7t_trt.get(
+        subject='01', ses='1', suffix='T1w', acquisition=acq)
     assert len(t1w_files) == 1
-    bold_files = layout_7t_trt.get(subject='01', ses='1', suffix='bold', acquisition=acq)
+    bold_files = layout_7t_trt.get(
+        subject='01', ses='1', suffix='bold', acquisition=acq)
     assert len(bold_files) == 0
 
 
 def test_get_val_enum_any(layout_7t_trt):
-    t1w_files = layout_7t_trt.get(subject='01', ses='1', suffix='T1w', acquisition=Query.ANY)
+    t1w_files = layout_7t_trt.get(
+        subject='01', ses='1', suffix='T1w', acquisition=Query.ANY)
     assert not t1w_files
     bold_files = layout_7t_trt.get(subject='01', ses='1', run=1, suffix='bold',
                                    acquisition=Query.ANY)
@@ -264,8 +265,8 @@ def test_ignore_files(layout_ds005):
     assert target1 not in layout1.files
     assert target2 not in layout1.files
     # now the models/ dir should show up, because passing ignore explicitly
-    # overrides the default - but 'model/extras/' should still be ignored because
-    # of the regex.
+    # overrides the default - but 'model/extras/' should still be ignored
+    # because of the regex.
     ignore = [re.compile('xtra'), 'dummy']
     layout2 = BIDSLayout(data_dir, validate=False, ignore=ignore)
     assert target1 in layout2.files
@@ -274,7 +275,7 @@ def test_ignore_files(layout_ds005):
 
 def test_force_index(layout_ds005):
     data_dir = join(get_test_data_path(), 'ds005')
-    target= join(data_dir, 'models', 'ds-005_type-test_model.json')
+    target = join(data_dir, 'models', 'ds-005_type-test_model.json')
     model_layout = BIDSLayout(data_dir, validate=True, force_index=['models'])
     assert target not in layout_ds005.files
     assert target in model_layout.files
@@ -290,7 +291,7 @@ def test_nested_include_exclude():
 
     # Nest a directory exclusion within an inclusion
     layout = BIDSLayout(data_dir, validate=True, force_index=['models'],
-                      ignore=[os.path.join('models', 'extras')])
+                        ignore=[os.path.join('models', 'extras')])
     assert layout.get_file(target1)
     assert not layout.get_file(target2)
 
@@ -303,7 +304,7 @@ def test_nested_include_exclude():
     # Force file inclusion despite directory-level exclusion
     models = ['models', target2]
     layout = BIDSLayout(data_dir, validate=True, force_index=models,
-                      ignore=[os.path.join('models', 'extras')])
+                        ignore=[os.path.join('models', 'extras')])
     assert layout.get_file(target1)
     assert layout.get_file(target2)
 
@@ -441,7 +442,8 @@ def test_parse_file_entities():
     target = {'subject': '03', 'session': '07', 'run': 4, 'suffix': 'sekret',
               'desc': 'bleargh', 'extension': 'nii.gz'}
     assert target == parse_file_entities(filename)
-    assert target == parse_file_entities(filename, config=['bids', 'derivatives'])
+    assert target == parse_file_entities(
+        filename, config=['bids', 'derivatives'])
 
     # Test with list of Entities
     entities = [
@@ -479,60 +481,6 @@ def test_parse_file_entities_from_layout(layout_synthetic):
     target = {'desc': 'bleargh'}
     assert target == layout.parse_file_entities(filename, config='derivatives')
 
-def test_parse_file_entities_from_layout_cached_db(layout_synthetic_cached_db):
-    layout = layout_synthetic_cached_db
-    filename = '/sub-03_ses-07_run-4_desc-bleargh_sekret.nii.gz'
-
-    # Test with entities taken from bids config
-    target = {'subject': '03', 'session': '07', 'run': 4, 'suffix': 'sekret',
-              'extension': 'nii.gz'}
-    assert target == layout.parse_file_entities(filename, config='bids')
-    config = Config.load('bids')
-    assert target == layout.parse_file_entities(filename, config=[config])
-    assert target == layout.parse_file_entities(filename, scope='raw')
-
-    # Test with default scope--i.e., everything
-    target = {'subject': '03', 'session': '07', 'run': 4, 'suffix': 'sekret',
-              'desc': 'bleargh', 'extension': 'nii.gz'}
-    assert target == layout.parse_file_entities(filename)
-    # Test with only the fmriprep pipeline (which includes both configs)
-    assert target == layout.parse_file_entities(filename, scope='fmriprep')
-    assert target == layout.parse_file_entities(filename, scope='derivatives')
-
-    # Test with only the derivative config
-    target = {'desc': 'bleargh'}
-    assert target == layout.parse_file_entities(filename, config='derivatives')
-
-
-def test_parse_file_entities_from_layout_cached_db_replay(layout_synthetic_cached_db_replay):
-    """
-    We need to ensure that after caching the database requesting cached values
-    provides all the original values.
-    :param layout_synthetic_cached_db_replay:
-    """
-    layout = layout_synthetic_cached_db_replay
-    filename = '/sub-03_ses-07_run-4_desc-bleargh_sekret.nii.gz'
-
-    # Test with entities taken from bids config
-    target = {'subject': '03', 'session': '07', 'run': 4, 'suffix': 'sekret',
-              'extension': 'nii.gz'}
-    assert target == layout.parse_file_entities(filename, config='bids')
-    config = Config.load('bids')
-    assert target == layout.parse_file_entities(filename, config=[config])
-    assert target == layout.parse_file_entities(filename, scope='raw')
-
-    # Test with default scope--i.e., everything
-    target = {'subject': '03', 'session': '07', 'run': 4, 'suffix': 'sekret',
-              'desc': 'bleargh', 'extension': 'nii.gz'}
-    assert target == layout.parse_file_entities(filename)
-    # Test with only the fmriprep pipeline (which includes both configs)
-    assert target == layout.parse_file_entities(filename, scope='fmriprep')
-    assert target == layout.parse_file_entities(filename, scope='derivatives')
-
-    # Test with only the derivative config
-    target = {'desc': 'bleargh'}
-    assert target == layout.parse_file_entities(filename, config='derivatives')
-
 
 def test_deriv_indexing():
     data_dir = join(get_test_data_path(), 'ds005')
@@ -609,8 +557,9 @@ def test_indexed_file_associations(layout_7t_trt):
     targets = [
         os.path.join(layout_7t_trt.root,
                      'sub-01/ses-1/fmap/sub-01_ses-1_run-1_phasediff.nii.gz'),
-        os.path.join(img.dirname,
-                     'sub-01_ses-1_task-rest_acq-fullbrain_run-1_physio.tsv.gz'),
+        os.path.join(
+            img.dirname,
+            'sub-01_ses-1_task-rest_acq-fullbrain_run-1_physio.tsv.gz'),
         os.path.join(layout_7t_trt.root, 'task-rest_acq-fullbrain_bold.json')
     ]
     assert set([a.path for a in assocs]) == set(targets)
@@ -622,15 +571,14 @@ def test_indexed_file_associations(layout_7t_trt):
     assert not js.get_associations('InformedBy')
 
 
-def test_layout_save(layout_7t_trt):
-    _, f = tempfile.mkstemp(suffix='.db')
-    layout_7t_trt.save(f, replace_connection=False)
+def test_layout_save(tmp_path, layout_7t_trt):
+    layout_7t_trt.save(str(tmp_path / "f.sqlite"),
+                       replace_connection=False)
     data_dir = join(get_test_data_path(), '7t_trt')
-    layout = BIDSLayout(data_dir, database_file=f)
+    layout = BIDSLayout(data_dir, database_path=str(tmp_path))
     oldfies = set(layout_7t_trt.get(suffix='events', return_type='file'))
     newfies = set(layout.get(suffix='events', return_type='file'))
     assert oldfies == newfies
-    os.unlink(f)
 
 
 def test_indexing_tag_conflict():