ENH: Add expert option to load BIDS layouts from database file #187

Shotgunosine · 2019-09-27T17:55:55Z

Closes #186

effigies · 2019-09-27T18:03:01Z

You're going to want to add it to these places, as well:

fitlins/fitlins/utils/bids.py

Line 75 in 1e45226

layout = BIDSLayout(bids_dir)

fitlins/fitlins/interfaces/bids.py

Line 107 in 4fea54e

layout = bids.BIDSLayout(self.inputs.bids_dir, validate=False)

fitlins/fitlins/interfaces/bids.py

Lines 230 to 231 in 4fea54e

    
           layout = BIDSLayout(self.inputs.bids_dir, force_index=force_index, 
        
                               ignore=ignore, derivatives=derivatives)

fitlins/fitlins/interfaces/bids.py

Line 421 in 4fea54e

layout = BIDSLayout(self.inputs.bids_dir, derivatives=derivatives)

And this layout should be built with derivatives.

codecov-io · 2019-09-27T18:12:18Z

Codecov Report

Merging #187 into master will decrease coverage by 0.02%.
The diff coverage is 82.14%.

@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
- Coverage   76.51%   76.48%   -0.03%     
==========================================
  Files          18       18              
  Lines        1026     1029       +3     
  Branches      179      181       +2     
==========================================
+ Hits          785      787       +2     
- Misses        149      150       +1     
  Partials       92       92

Flag	Coverage Δ
#ds003	`76.48% <82.14%> (-0.03%)`	⬇️

Impacted Files	Coverage Δ
fitlins/utils/bids.py	`44.64% <100%> (-0.98%)`	⬇️
fitlins/interfaces/bids.py	`72.96% <62.5%> (-1.14%)`	⬇️
fitlins/workflows/base.py	`60.21% <75%> (-0.23%)`	⬇️
fitlins/cli/run.py	`84.49% <93.33%> (+2.14%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d2675c...da3031d. Read the comment docs.

Shotgunosine · 2019-09-30T17:19:22Z

Alright, trying to add the database_file option to those interfaces. Is there a preferred way to specify an optional input that should be an existing file path if provide, but be None otherwise?

effigies · 2019-09-30T17:22:12Z

This or similar:

if database_file is not None:
    interface.inputs.database_file = database_file

Shotgunosine · 2019-09-30T17:24:48Z

Something like this should work for specifying it in the input spec?

class ModelSpecLoaderInputSpec(BaseInterfaceInputSpec):
    bids_dir = Directory(exists=True,
                         mandatory=True,
                         desc='BIDS dataset root directory')
    database_file = traits.File(exists=True,
                                desc='Optional path to bids database file.')
    model = traits.Either('default', InputMultiPath(File(exists=True)),
                          desc='Model filename')
    selectors = traits.Dict(desc='Limit models to those with matching inputs')

effigies · 2019-09-30T17:26:00Z

Yes.

effigies

Overall looks reasonable.

fitlins/workflows/base.py

effigies · 2019-09-30T21:05:37Z

If this works for you, I'm good to merge it. It would be good to add tests at some point, but most of the lines we're currently missing require iterating over BIDS datasets with different properties, which makes it a finicky proposition.

Shotgunosine · 2019-10-01T11:31:08Z

I've currently only tested that the code still works if you don't pass a database file, I'll try to test functionality when passed a database file sometime today and add it to the CircleCI config. I've got an abstract to write today though, so I may not get to it till later on in the week.

pep8speaks · 2019-10-02T16:13:08Z

Hello @Shotgunosine, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, pip install flake8 and then run flake8 fitlins.

Comment last updated at 2019-12-06 20:14:23 UTC

effigies · 2019-10-02T16:13:52Z

@Shotgunosine Heads up that I resolved merge conflicts, so you'll want to pull before any further work.

effigies

Minor style changes.

fitlins/cli/run.py

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

effigies · 2019-11-14T23:23:10Z

Requires #197...

Shotgunosine · 2019-11-19T17:28:47Z

@effigies This is pretty much good to go. I think there's an issue with adding the code coverage file for the test I added. Do you think you could take a look at that?

effigies · 2019-11-19T17:35:42Z

Yup. I'll get to this ASAP.

adelavega · 2019-11-19T19:41:11Z

fitlins/cli/run.py

    g_bids.add_argument('--force-index', action='store', default=None,
                        help='regex pattern or string to include files')
    g_bids.add_argument('--ignore', action='store', default=None,
                        help='regex pattern or string to ignore files')
    g_bids.add_argument('--desc-label', action='store', default='preproc',
                        help="use BOLD files with the provided description label")
+    g_bids.add_argument('--database-path', action='store', default=None,
+                        help="Caution, this is an Expert level option subject to change! "


Not sure if this caution is necessary given how few people use this, and how it's overall an early stage project.

Yeah, this is turning into less of a one-off hack. I guess we can remove the warning.

adelavega · 2019-11-19T19:49:10Z

fitlins/cli/run.py

+        reset_database = True
+        make_layout = True
+    elif Path(opts.database_path).exists():
+        layout = BIDSLayout.load(opts.database_path)


It might be better to let pybids handle some of this logic.

That is, is path exists, let pybids decide to load the database from it.
The problem with the current approach is that if someone passes a valid db path, but then changes the CLIs arguments (so that they would actually want to re-index), no error will be thrown. So their CLI options are being silently ignored in favor for the indexing options in the db_path.

I rather let pybids crash if the options mismatch. Also simplifies this logic.

adelavega · 2019-11-19T19:54:53Z

fitlins/cli/run.py

+    # Go ahead and initialize the layout database
+    if opts.database_path is None:
+        database_path = Path(work_dir) / 'dbcache'
+        reset_database = True


Do we need to manually ask pybids to reset_database? It will do it on its own if it doesn't file a valid sqlite file inside the database_path.

I suppose this is safer though, as it could be a mismatching dbcache from another run.

An alternative here is to clean up the dbcache folder on fitlins teardown. (and put it somewhre more hidden, at least make it .dbcache). Or use a tmpdir, although @effigies seemed to think that would be a problem (not guaranted to be on disk).

I think the right solution is to use tempfile.TemporaryDirectory but set prefix to work_dir/.dbcache. That way its cleaned up automatically. Or can also be cleaned up manually with ``cleanup` function.

I suppose this is safer though, as it could be a mismatching dbcache from another run.

Yes, that's the purpose here. If somebody passes a db file, then we assume they know that it's valid. If we're creating it, we don't want to take any chances, and will always clear it.

I think the right solution is to use tempfile.TemporaryDirectory but set prefix to work_dir/.dbcache. That way its cleaned up automatically.

That would be fine, but I tend to think of anything in the scratch directory as disposable anyway.

That's true. Maybe I'm overthinking this then, and it might actually be useful to have the index there for debugging.

Maybe I'm over complicating things these, but I could imagine it being useful to not force re-index, when you re-run fitlins several times. I guess you could just build it yourself and pass the path in, but its nice to not have to manage that.

Feel free to ignore this suggestion.

I deffo want it for debugging, and this way, if they want to reuse a layout saved in a working directory, they can pass the path for it explicitly, otherwise it gets reset in order to avoid nasty surprises.

fitlins/interfaces/bids.py

effigies · 2019-11-19T20:41:17Z

fitlins/cli/run.py

    g_bids.add_argument('--force-index', action='store', default=None,
                        help='regex pattern or string to include files')
    g_bids.add_argument('--ignore', action='store', default=None,
                        help='regex pattern or string to ignore files')
    g_bids.add_argument('--desc-label', action='store', default='preproc',
                        help="use BOLD files with the provided description label")
+    g_bids.add_argument('--database-path', action='store', default=None,
+                        help="Caution, this is an Expert level option subject to change! "
+                             "Path to directory containing SQLite database indicies "


Suggested change

"Path to directory containing SQLite database indicies "

"Path to directory containing SQLite database indices "

effigies · 2019-11-19T20:45:45Z

fitlins/cli/run.py

    g_bids.add_argument('--force-index', action='store', default=None,
                        help='regex pattern or string to include files')
    g_bids.add_argument('--ignore', action='store', default=None,
                        help='regex pattern or string to ignore files')
    g_bids.add_argument('--desc-label', action='store', default='preproc',
                        help="use BOLD files with the provided description label")
+    g_bids.add_argument('--database-path', action='store', default=None,
+                        help="Caution, this is an Expert level option subject to change! "


Yeah, this is turning into less of a one-off hack. I guess we can remove the warning.

effigies · 2019-11-19T21:05:17Z

fitlins/cli/run.py

+    # Go ahead and initialize the layout database
+    if opts.database_path is None:
+        database_path = Path(work_dir) / 'dbcache'
+        reset_database = True


I suppose this is safer though, as it could be a mismatching dbcache from another run.

Yes, that's the purpose here. If somebody passes a db file, then we assume they know that it's valid. If we're creating it, we don't want to take any chances, and will always clear it.

I think the right solution is to use tempfile.TemporaryDirectory but set prefix to work_dir/.dbcache. That way its cleaned up automatically.

That would be fine, but I tend to think of anything in the scratch directory as disposable anyway.

effigies · 2019-11-19T21:06:37Z

fitlins/cli/run.py

+        reset_database = True
+        make_layout = True
+    elif Path(opts.database_path).exists():
+        layout = BIDSLayout.load(opts.database_path)


effigies · 2019-11-19T21:08:29Z

fitlins/cli/run.py

+    if opts.participant_label is not None:
+        subject_list = bids.collect_participants(
+            opts.bids_dir, participant_label=opts.participant_label,
+            database_path=database_path)


Should we just pass the layout, rather than instantiating another one in the function?

Looks like it

effigies · 2019-11-19T21:09:21Z

fitlins/cli/run.py

@@ -196,7 +229,7 @@ def run_fitlins(argv=None):
        except Exception:
            retcode = 1

-    layout = BIDSLayout(opts.bids_dir, derivatives=derivatives)
+    layout = BIDSLayout.load(database_path)


Now that layout is guaranteed to exist, I think we can just reuse it?

Suggested change

layout = BIDSLayout.load(database_path)

fitlins/interfaces/bids.py

effigies · 2019-11-19T21:20:03Z

fitlins/interfaces/bids.py

@@ -142,6 +142,8 @@ class LoadBIDSModelInputSpec(BaseInterfaceInputSpec):
                         desc='BIDS dataset root directory')
    derivatives = traits.Either(traits.Bool, InputMultiPath(Directory(exists=True)),
                                desc='Derivative folders')
+    database_path = Directory(exists=False,
+                              desc='Optional path to bids database directory.')


Here, you're setting this up as an alternative to bids_dir/derivatives, but in _run_interface, you're replacing them entirely. We should be consistent about which one we're doing.

Would you prefer it to be optional or to have LoadBIDSModel and BIDSSelect only work if passed a database_path?

Optional at the CLI level for sure. But we could set up a default internal path (don't we already do that?).

effigies · 2019-11-19T21:21:01Z

fitlins/interfaces/bids.py

@@ -395,6 +382,8 @@ class BIDSSelectInputSpec(BaseInterfaceInputSpec):
                         desc='BIDS dataset root directories')
    derivatives = traits.Either(True, InputMultiPath(Directory(exists=True)),
                                desc='Derivative folders')
+    database_path = Directory(exists=False,
+                              desc='Optional path to bids database path.')


Again. Inputs say optional, runtime says replace.

setup.cfg

effigies · 2019-12-04T19:32:38Z

@Shotgunosine Just a reminder that this one's waiting on you.

Shotgunosine · 2019-12-05T20:31:11Z

Yeah, thanks, have not had time, but will try to get to it soon.

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

Shotgunosine · 2019-12-06T21:14:56Z

Alright, changes made. Let me know if there's anything else to tweak.

effigies · 2019-12-06T21:57:42Z

Thanks for pushing on this. Today was slow (sick kid), but I'll try to get to this on Monday.

effigies · 2019-12-09T22:06:59Z

LGTM. Thanks for your patience.

Shotgunosine added 2 commits September 27, 2019 13:52

add database_file option

66e1dfd

fix database_file help message

43c3085

Shotgunosine added 2 commits September 30, 2019 14:35

add database_file to all interfaces that call BIDSlayout

908f347

Merge branch 'master' into database_file

69378c1

effigies reviewed Sep 30, 2019

View reviewed changes

fitlins/workflows/base.py Outdated Show resolved Hide resolved

fix syntax for adding database file to modelspecloader

496cde6

effigies changed the title ~~Database file~~ ENH: Add expert option to load BIDS layouts from database file Sep 30, 2019

effigies approved these changes Sep 30, 2019

View reviewed changes

Merge branch 'master' into database_file

c339d80

Shotgunosine added 2 commits November 11, 2019 16:09

Merge branch 'master' into database_file

d46d01b

make database file existance optional

dcc0dd9

effigies reviewed Nov 12, 2019

View reviewed changes

fitlins/cli/run.py Outdated Show resolved Hide resolved

fitlins/cli/run.py Outdated Show resolved Hide resolved

fitlins/cli/run.py Outdated Show resolved Hide resolved

Shotgunosine and others added 7 commits November 12, 2019 15:13

Fix pep8 issues

7a41c0f

Apply suggestions from code review

d057f0a

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

Add database-file test to circleci config

dd73459

fix circleci config

87894a1

tweak circleci config

e0f50b5

MNT: Pin pybids branch

7f25ed0

Update setup.cfg

adb0cf6

Shotgunosine added 4 commits November 18, 2019 16:26

reset multiproc settings

f7bbe4b

FIX running without database file

db95ca8

FIX circleci config

33ca0cc

FIX pep8 issues

0f6788b

effigies added this to Waiting on @effigies in Priority Nov 19, 2019

Shotgunosine added 3 commits November 19, 2019 13:02

FIX coverage

3aea9c6

check behavior if database path exists

7814115

check behavior if database path exists

009ebe0

adelavega requested changes Nov 19, 2019

View reviewed changes

effigies reviewed Nov 19, 2019

View reviewed changes

effigies moved this from Waiting on @effigies to Waiting on submitter in Priority Nov 19, 2019

effigies reviewed Nov 20, 2019

View reviewed changes

setup.cfg Outdated Show resolved Hide resolved

Merge branch 'master' into database_file

569cb47

Shotgunosine and others added 7 commits December 5, 2019 15:45

Update setup.cfg

81adc56

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

simplifiy load logic

ec6e651

Pass layout instead of instantiating one

604f5d5

don't reload the database

2595f70

make interfaces take only database path

5d4a991

remove make_layout

4c89081

Fix database_path description

da3031d

effigies merged commit ad0a153 into poldracklab:master Dec 9, 2019

adelavega mentioned this pull request Dec 10, 2019

Ignore is missing from LoadBIDSModelInputSpec #201

Closed

jdkent mentioned this pull request Dec 17, 2019

improve pybids indexing speed HBClab/NiBetaSeries#252

Closed

	"Path to directory containing SQLite database indicies "
	"Path to directory containing SQLite database indices "

ENH: Add expert option to load BIDS layouts from database file #187

ENH: Add expert option to load BIDS layouts from database file #187

Conversation

Shotgunosine commented Sep 27, 2019

effigies commented Sep 27, 2019

codecov-io commented Sep 27, 2019 • edited

Codecov Report

Shotgunosine commented Sep 30, 2019

effigies commented Sep 30, 2019 • edited

Shotgunosine commented Sep 30, 2019 • edited

effigies commented Sep 30, 2019

effigies left a comment

Choose a reason for hiding this comment

effigies commented Sep 30, 2019

Shotgunosine commented Oct 1, 2019

pep8speaks commented Oct 2, 2019 • edited

Comment last updated at 2019-12-06 20:14:23 UTC

effigies commented Oct 2, 2019

effigies left a comment

Choose a reason for hiding this comment

effigies commented Nov 14, 2019

Shotgunosine commented Nov 19, 2019

effigies commented Nov 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adelavega Nov 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Dec 4, 2019

Shotgunosine commented Dec 5, 2019

Shotgunosine commented Dec 6, 2019

effigies commented Dec 6, 2019

effigies commented Dec 9, 2019

codecov-io commented Sep 27, 2019 •

edited

effigies commented Sep 30, 2019 •

edited

Shotgunosine commented Sep 30, 2019 •

edited

pep8speaks commented Oct 2, 2019 •

edited

adelavega Nov 19, 2019 •

edited