Close FITS files at the end of Gammapy read functions? #1055

This issue is very similar to #539, but I wanted to split out this question into a separate discussion / issue.

The short version of my question is (mainly to @woodmd and @kosack , but anyone can chime in, maybe @adonath or @joleroi ): OK to change to this everywhere in Gammapy read methods to make sure files are closed?

with fits.open(filename) as hdu_list

Here's the long version:

While looking at #1054 I saw warnings from the tests:

  /private/var/folders/sb/4qv5j4m90pz1rw7m70rj1b1r0000gn/T/gammapy-test-wnlxau5o/lib.macosx-10.12-x86_64-3.6/gammapy/maps/tests/test_hpxcube.py:51: ResourceWarning: unclosed file <_io.FileIO name='/private/var/folders/sb/4qv5j4m90pz1rw7m70rj1b1r0000gn/T/pytest-of-deil/pytest-18/test_hpxcube_read_write_8_Fals0/skycube.fits' mode='rb' closefd=True>
    m2 = HpxMapND.read(filename)

This made me think: What behaviour do we want for classes that do FITS I/O?
Should functions like SkyCube.read or EventList.read always close FITS files before returning, or keep file handles open?

The simplest solution certainly would be if file I/O is always limited to read and write methods, and those methods never keep files open.

The question is: do we have use cases where we need to keep files open and avoid doing the complete read and file close in the read methods, to limit RAM use or to support analyses that "lazy-load" only parts of arrays (via mmap which Numpy and astropy.io.fits support) needed at later times?

From the applications / uses of Gammapy I've seen, I would say it's OK to have the simple solution / codebase / testing and to not support handing around of open FITS files within Gammapy.

Currently many all of our read functions (such as SkyImage.read or SkyCube.read do something equivalent to this:

@classmethod
def read(cls, filename):
    hdu_list = fits.open(filename)
   return cls(data=hdu_list[0].data)

Reading http://docs.astropy.org/en/stable/io/fits/appendix/faq.html#i-m-opening-many-fits-files-in-a-loop-and-getting-oserror-too-many-open-files I'm not exactly sure what exactly happens with the open file handle, i.e. whether usually hdu_list gets deleted and the file closed after a while.

But the point is: if we don't need this, then we could just change to

@classmethod
def read(cls, filename):
    with fits.open(filename) as hdu_list:
        data = hdu_list[0].data
   return cls(data=data)

i.e. use the usual Python pattern to make sure files get closed by using a with block, which leads to hdu_list.__exit__() being called, which contains one line self.close().
Again, reading http://docs.astropy.org/en/stable/io/fits/appendix/faq.html#i-m-opening-many-fits-files-in-a-loop-and-getting-oserror-too-many-open-files I'm not sure if that always closes the file, but I think usually it does and leads to better / more deterministic behaviour?

Looking around what others do, these guys use with fits.open:
https://github.com/astropy/ccdproc/blob/a3f6ccb710989a6f3caf1de98b249336b37d32bd/ccdproc/ccddata.py#L674
and astropy.table.Table.read eventually calls into
https://github.com/astropy/astropy/blob/c4d4060a265350198f7bd863ba59ec71a8b2ddf9/astropy/io/fits/connect.py#L135

        try:
            return read_table_fits(hdulist, hdu=hdu)
        finally:
            hdulist.close()

which I think is equivalent to with open() as hdu_list, i.e. another idiom to make sure hdu_list.close() always gets called.

So in short: I'm proposing to change to with fits.open() as hdu_list and close files at the end of read methods everywhere in Gammapy.