Table.read on FITS HDU leaves files open

@saimn

In Gammapy we still have issues in some cases when opening many ( ~ 1000 ) small tables from FITS files and reading them, leading to:

OSError: [Errno 24] Too many open files

We only support analyses where all data fits in memory, and have changed all our code to use (see gammapy/gammapy#1055) with fits.open in the read methods of our classes, but in some cases FITS files aren't closed at the end of the with block as expected.

The issue only appears if we call Table.read on HDU objects inside the with block.
Here is a minimal test case to reproduce (at least for me on Astropy master on Mac OS)

from astropy.io import fits
from astropy.table import Table
from psutil import Process

table = Table(dict(a=[1, 2, 3]))
table.write('test.fits', overwrite=True)
print(Process().open_files())

with fits.open('test.fits') as hdu_list:
    t2 = Table.read(hdu_list[1])

print(Process().open_files())

results in

[]
[popenfile(path='/tmp/test.fits', fd=4)]

i.e. the FITS file is still open after the with block ends.

I'm not sure exactly what is going on, some observations:

hdu_list._file shows the file as closed, but psutil shows it as open
I'm not sure if the file is closed eventually when the Python cyclic reference detection from the garbage collector kicks in. I think so, but still the issue remains that some Gammapy analyses run out of files, and in this simple example the FITS file isn't closed reliably.
The problem must have to do with the operations performed by Table.read, if I remove that line then the with fits.open reliably closes the file at the end of the with block.

@saimn @taldcroft or anyone - can you reproduce? do you understand what is going on?

This seems related to memory mapping - can you try and fits.open with memmap=False?

If it is indeed memmap=True, please look at the warning under http://docs.astropy.org/en/stable/io/fits/#working-with-large-files .

Alternative is to up your ulimit for max open files, but this only works if you have enough memory to handle the new limit. Ref: https://stackoverflow.com/questions/34588/how-do-i-change-the-number-of-open-files-limit-in-linux

@pllim

@pllim - Thanks!

Indeed, with memmap=False the issue disappears, the file is closed. Maybe we will change to use memmap=False everywhere in Gammapy.

But still, why is the file left open if I do Table.read(hdu_list[1]), but closed e.g. if I just access hdu_list[1].data? Shouldn't Table.read(hdu_list[1]) in this case make a copy of the data in this case, and not hold a reference to hdu_list[1].data after the Table.read call exits? I do see the same problem of the memmap file not closing on with block exit even if there's just a Table(hdu_list[1].data, copy=True) in the block. Is this expected behaviour can Table be changed to behave more nicely here?

I think we need to be careful here to not make it copy too much - i.e. there are some cases when it's fine for:

with fits.open('test.fits') as hdu_list:
    t2 = Table.read(hdu_list[1])

to then have a table that has memory mapped arrays as you are seeing. I guess the question here is - are the files finally closed when the table is garbage collected? Are you keeping references to the 1000 tables or are you e.g stacking them? Could you provide a minimal example of how you are looping over all the tables?

Just to put some ideas out there, one option would be that we can detect when more than a certain number of FITS files are opened with memory mapping and give a warning like the matplotlib warning when too many figures are open. I'll try and think of some other options but it would be helpful to have more information about your real use case.

My understanding is that FITS stores data on disk row by row, and Table transforms on read to column by column, i.e. there is a copy of the data anyways and it doesn't make sense for Table to hold on to the memmapped file. Is this not true?

Here's a self-contained example illustrating basically how we process data in Gammapy:

from pprint import pprint
from astropy.io import fits
from astropy.table import Table
from psutil import Process

n_tables = 100

# Create test files
# This is just to illustrate a file open / memmap issue
# content doesn't matter, so we just always put the same
for idx in range(n_tables):
    table = Table(dict(a=[1, 2, 3]))
    filename = 'test_{:06d}.fits'.format(idx)
    print('Writing {}'.format(filename))
    table.write(filename, overwrite=True)
    pprint(Process().open_files())

# Process many tables; we want this to reliably
# close the files after processing each one
result = 0

for idx in range(n_tables):
    filename = 'test_{:06d}.fits'.format(idx)
    print('Reading {}'.format(filename))
    with fits.open(filename) as hdu_list:
        table = Table.read(hdu_list[1])
        result += table['a'].sum()
    pprint(Process().open_files())

print('All done. Open files:')
pprint(Process().open_files())

If I run this with the default memmap=False, then some files remain open and some are closed:
https://gist.github.com/cdeil/0d7e3de3a2e17b38e18be5260f08c6cf#file-gistfile1-txt-L1811-L1840

So the argument that Table needs to hold the memmap open isn't valid, because it's not consistently held open, no?

When using memmap=False you should never see the file opened after the with block. Also Table.read use memmap=False by default, since v3.0 and #6821.

But if you don't use memmap=False, creating the Table add references to the hdu.data object, so when closing the file HDUList.close see these references and does not close the memmap object (I think this is the reason).

With memmap, the only way I found to really close the file is to copy the table and call the garbage collector, but it's easier to disable memmap ;)

with fits.open('test.fits') as hdu_list:
    t2 = Table.read(hdu_list[1]).copy()
    gc.collect()

Thank you all for your help and comments here!

I did change to with fits.open(filename, memmap=False) in Gammapy everywhere in gammapy/gammapy#1407 and that resolves the issue, as others have said with memmap=False files are closed reliably at the end of the with block.

But there is one thing left here I don't understand:

But if you don't use memmap=False, creating the Table add references to the hdu.data object

Why?

As I mentioned above my understanding is that Table stores the data column-wise, and FITS row-wise, so there always has to be a copy of the data and Table should not need to still hold on to a reference of hdu.data when Table.read exits, no?

There is definitely a bug here but it's an astropy.table bug, not io.fits. Consider the following example:

from psutil import Process
import numpy as np
from numpy import memmap as Memmap


with open('data', 'wb') as f:
    f.write(b'\x00' * 256)

for iter in range(200):
    m = Memmap('data', np.float)

m = None

print(len(Process().open_files()))

this gives 0. Now consider this:

from psutil import Process
import numpy as np
from numpy import memmap as Memmap
from astropy.table import Table


with open('data', 'wb') as f:
    f.write(b'\x00' * 256)

for iter in range(200):
    m = Memmap('data', np.float)
    t = Table()
    t['a'] = m

m = t = None

print(len(Process().open_files()))

This returns 49! So clearly there is some kind of issue with the garbage collection in Table.

Note that doing:

for iter in range(200):
    m = Memmap('data', np.float)
    c = Column(data=m)

works fine and doesn't leave files open, so this is something to do with Table I think.

As I mentioned above my understanding is that Table stores the data column-wise, and FITS row-wise, so there always has to be a copy of the data and Table should not need to still hold on to a reference of hdu.data when Table.read exits, no?

The whole table data is memory mapped, and colums are then strided views on that array, so memory mapping does work for tables I think.

Note that memmap defaults to False in Table.read when reading from a FITS file:

https://github.com/astropy/astropy/blob/master/astropy/io/fits/connect.py#L115

So you'd be better off doing:

    table = Table.read(filename, hdu=1)

instead of

    with fits.open(filename) as hdu_list:
        table = Table.read(hdu_list[1])

So just for the record, to summarize what I said in #7404, There are two independent things going on here:

I think it's normal that:

with fits.open('test.fits') as hdu_list:
    t2 = Table.read(hdu_list[1])

leads to an open file even after the with block as t2 needs to be able to continue functioning with the memory-mapped arrray

When done inside a loop:

for iter in range(200):
    with fits.open('test.fits') as hdu_list:
        t2 = Table.read(hdu_list[1])

It looks like the memory-mapped arrays are not always garbage collected at the end of each iteration, which is a bug/issue.

I think I may have tracked it down - more soon

@cdeil

@cdeil @saimn - see #7447, I think that this is what's causing the memory mapped arrays to not be garbage collected.

@cdeil

@cdeil - so just to be clear, now that #7448 does reliably close memory maps when they go out of scope, I don't think there's anything else we can do in terms of gammapy - I don't think we can make the memory maps stop working after the FITS file is closed because at that point the arrays are numpy ndarrays and we don't control the underlying buffer, so as long as you keep hold of the data objects then the memory maps will remain open.

So if you want to avoid that then turning off the memory mapping is the correct solution, or you could instead do an explicit copy of the data.

@astrofrog

The whole table data is memory mapped, and colums are then strided views on that array, so memory mapping does work for tables I think.

Interesting.

I see, so Table write to FITS and read back it comes back with a completely different memory layout:
https://gist.github.com/cdeil/f6a63f9109a66d8f38056364d2b0ef04

This might have a large performance impact for packages like Gammapy that process data in a columnar fashion. Making a copy on read might be a good idea to get data column-wise and then during analysis have good memory / cache performance when processing the data (e.g. histogramming events into sky images, or applying event selections e.g. on time or energy). Will have to investigate ...

Do you know why in the last example there the EVENT_ID column has a stride of 4? Shouldn't it be 36 like all other columns!???

EVENT_ID 4 (4,) True

So if you want to avoid that then turning off the memory mapping is the correct solution, or you could instead do an explicit copy of the data.

I think you're right.

Note that I did change to memmap=False a few days ago in Gammapy.

Does it ever make sense (e.g. for tables or images, small or large), to open with memmap=True and make a copy to have the data in the nice in-memory form that we want? Or does memmap=True not help if all you do is make a copy and then close the file?

I'm still confused when exactly the data is read from disk into memory and if / how often a copy is made ... I'll have another look at this and try to fully understand it in the coming days.

@astrofrog - Up to you if you want to close this issue now, or leave it open until the code and docs improvements are merged into master.

Can we close this one also now that #7448 is merged ?

@cdeil

@cdeil can you please try your use case with latest release?

@cdeil

@cdeil @pllim - Please use the dev version, the fix hasn't been included in the latest release (given that we are "wait and see" for those occasional CI failures.)

Ops, yes. I forgot 2.0.8 is not released yet. My bad, sorry!

@astrofrog

@astrofrog @saimn - Thank you!

I have a new Mac and am not able to reproduce the issue any more with Anaconda Python.

But given that the memory leak was fixed, it should be gone.

Closing now.

Both with current Astropy master (27244c4) and with Astropy 3.0.2 I see the expected behaviour (only one file remains open, and files seem to be closed as the tables that read from them go out of scope in the for loop) with this test script:

from astropy.io import fits
from astropy.table import Table
from psutil import Process

import astropy
print(astropy.__version__)
print(astropy.version.githash)

table = Table(dict(a=[1, 2, 3]))
table.write('test.fits', overwrite=True)
print(Process().open_files())

for _ in range(200):
    with fits.open('test.fits') as hdu_list:
        t2 = Table.read(hdu_list[1])

print(Process().open_files())

Navigation Menu

Repository navigation

Table.read on FITS HDU leaves files open #7440

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Table.read on FITS HDU leaves files open #7440

Description

Activity

astrofrog commented on May 6, 2018

pllim commented on May 6, 2018

cdeil commented on May 6, 2018

astrofrog commented on May 7, 2018

cdeil commented on May 7, 2018

saimn commented on May 8, 2018

saimn commented on May 8, 2018

cdeil commented on May 8, 2018

astrofrog commented on May 10, 2018

astrofrog commented on May 10, 2018

astrofrog commented on May 10, 2018

astrofrog commented on May 10, 2018

astrofrog commented on May 10, 2018

astrofrog commented on May 10, 2018

astrofrog commented on May 10, 2018

cdeil commented on May 10, 2018

saimn commented on Jun 5, 2018

pllim commented on Jun 6, 2018

bsipocz commented on Jun 6, 2018

pllim commented on Jun 6, 2018

cdeil commented on Jun 8, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions