Intent to deprecate: keep_file_open

[effigies]

As of indexed_gzip 0.7.0, the default behavior of a SafeIndexedGzipFile is to close the underlying file between accesses. This pretty much obviates the machinery @pauldmccarthy built around the keep_file_open option in Opener, ArrayProxy, AnalyzeImage and MGHImage.

As we did have a release, we're going to need to deprecate the API, but I think the following scheme should be reasonably safe:

• Set minimum indexed_gzip version to 0.7.0
• Remove any effects of keep_file_open - it only had any effect if indexed_gzip was installed AND keep_file_open was 'auto' or True - and update docs to say "has no effect, will be removed vXXX"
• Any function that currently takes keep_file_open raises a DeprecationWarning if it is not None.
• Remove arrayproxy.KEEP_FILE_OPEN_DEFAULT - This one I'm not sure of. I think people would only be setting it, not reading it, but maybe it should stay just in case.
• Schedule full removal of option for v4.0

What do y'all think?

[pauldmccarthy]

Hey @effigies, I don't have any strong feelings either way about this, apart from wondering whether there might be some use case (or future file IO driver) which depends on having a persistent file handle. This is not a problem for me personally though. Happy to help out if this goes ahead too!

[effigies]

I suppose that's a possibility, but I'm not sure what sort of case might exist where you couldn't emulate a persistently open filehandle by opening and seeking to last known location, prior to performing the operation.

In fact, perhaps what we want to do to get the current releasing behavior is a simple pass-through filehandle like the following (not fully worked through):

class ReleasingFileHandle:
    def __init__(self, file_like):
        self.file_like = file_like
        self._pos = 0 if not hasattr(file_like, tell) else file_like.tell()

    def _get_fileobj(self, mode):
        if isinstance(self.file_like, str):
            with open(self.file_like, mode) as fobj:
                fobj.seek(self._pos)
                yield fobj
                self._pos = fobj.tell()
        else:
            yield self.file_like

    def read(self, *args, **kwargs):
        with self._get_fileobj() as fobj:
            return fobj.read(*args, **kwargs)

    ...

Then as far as Opener and ArrayProxy are concerned, they always have an open filehandle. If passed another filehandle, then opening/closing between accesses is the responsibility of that filehandle. (As now, a normal filehandle would not be closed.)

This would be redoing work that's in indexed_gzip, and possibly some in Opener, so a little further refactoring might be called for, but this could eliminate the "create a new Opener unless keep_file_open" logic.

I may not be thinking this through too clearly. As you've dug through these bits of code more thoroughly than I have, does this seem plausible, or can you think of some alternative?

[pauldmccarthy]

I do like your ReleasingFileHandle idea - that would possibly make the existing code cleaner. Could this logic be built into the existing Opener class, and the ArrayProxy changed so that it persists a reference to a single Opener?

[effigies]

That might be the cleanest way of doing it. I had a brief look, but haven't had time to think carefully about how to do that. Feel free to push ahead, and I'll try to review. Otherwise I'll poke at it as I find time.

[pauldmccarthy]

Sure - I'm flying down under shortly so am quite busy, but I should have some time over the next couple of weeks.

[pauldmccarthy]

Hey @effigies, I've finally gotten around to working on this - #614 - let me know what you think!