BF: Fix interaction between indexed_gzip presence and keep_file_open flag
#679
Conversation
…re used, one to control persistence of ImageOpeners, another to control persistence of underlying file handles. This distinction is only relevant when indexed_gzip is being used.
pauldmccarthy
changed the title
indexed_gzip presence and keep_file_open flagindexed_gzip presence and keep_file_open flag
Codecov Report
@@ Coverage Diff @@
## master #679 +/- ##
==========================================
+ Coverage 88.87% 88.87% +<.01%
==========================================
Files 93 93
Lines 11444 11447 +3
Branches 1890 1891 +1
==========================================
+ Hits 10171 10174 +3
Misses 933 933
Partials 340 340
Continue to review full report at Codecov.
|
|
Hey @effigies, ready for a review - apologies if this changed your release plans! |
|
Thanks, Paul. I should have time today or tomorrow, and I'll shoot for a Monday release. We'll need to give Yarik time to run another round of tests on Neurodebian. Is Neurodebian up-to-date with the latest |
|
Neurodebian is at indexed_gzip 0.8.6, which is a tiny bit out of date - 0.8.7 contains mostly Windows-specific fixes, but also a small improvement in the read function. |
There was a problem hiding this comment.
Some very minor comments. This LGTM. Thanks for building out the logic in truth-table form. That makes things much simpler.
@matthew-brett I'll plan on merging Monday AM (PDT), in case you want to double-check the logic.
nibabel/tests/test_arrayproxy.py
Outdated
| assert not fobj1.closed | ||
| assert not fobj2.closed | ||
| except Exception: | ||
| print('Failed test', test) |
There was a problem hiding this comment.
Let's remove this debug print statement. If it's useful to add context, you can use six.raise_from to raise a new exception with the old one as context.
nibabel/tests/test_arrayproxy.py
Outdated
| fobj1.close() | ||
| fobj2.close() | ||
| # Test invalid values of keep_file_open | ||
| print('testinv') |
There was a problem hiding this comment.
Similarly, let's drop this print.
|
Thanks for the quick review! I had mean to remove those prints before pushing, they're gone now. The logic was doing my head in, so making that truth table was essential! |
|
Thanks for this. |
According to the discussion in #614, the behaviour of the
keep_file_openflag should be as follows (copied verbatim from this comment, which I believe was the behaviour that was agreed upon):But the behaviour of
nibabel2.3.0 doesn't match this expected behaviour.Assuming that
indexed_gzipis installed, a simple test shows that theArrayProxyis not creating and persisting oneImageOpener:This PR aims to fix the behaviour of
nibabelso that it matches what was discussed in #614.It does so by changing the internal logic of
ArrayProxyso that two flags are used:_persist_openerflag is used to control whether a singleImageOpeneris created and used for the lifetime of theArrayProxy, or whether a newImageOpeneris created for each file access_keep_file_openflag is used in a similar manner to control the lifetime of the underlying file handles.The distinction between these two flags is only relevant when
indexed_gzipis being used. The existingArrayProxycode dodes not correctly make this distinction.I've updated the unit tests in
test_arrayproxy.pyto make it clearer what scenarios are tested, and what the expected behaviour is for each scenario.