Memory leak reading and copying bin table FITS files with io.fits - #520

I would note that doing something like:

hdus = pyfits.open(filename)
dat = hdus[1].data
hdus.close()

is deceptive, since accessing the .data attribute by default opens a mmap to the file, so it's not "closed" in the sense that there are no open handles to the file. I generally don't call .close() unless I'm opening the file in a writeable mode, and even then I generally try to use the with statement.

I have a feeling this might be fixed and is just waiting in the wings for Astropy in #495. I'll double check on that though.

Oh, it seems I also misread your example. I thought it said that with .copy() there is no leak. That's a little more odd.

Confirmed in normal PyFITS as well. Also worth noting this has nothing to do with the fact that the file is gzipped, as I tried unzipping it first and had the same behavior.

This is a bit bizarre. But the FITS_rec class is a complicated ugly beast at best. I'm guessing that the .copy() isn't getting a full deep-copy of every object that makes up the table.

Sorry, I should have mentioned that I originally noted this issue in PyFITS. I originally used copy() for exactly the reason you said, namely that it seemed like a bad idea to directly reference to hdus[1].dat. The actual code where I noticed this collects up hundreds of such dat arrays and then stacks them up.

Apparently. this is still an issue. Sorry I haven't looked at this in so long. What triggers the issue has to do with accessing the individual columns of the table, not looking at their shapes. At least some of those columns need to be converted in the background from the native FITS format to a more useful format, and that happens in memory. That said, there still shouldn't be a leak. Somehow a reference cycle is getting made (thought I stomped those all out).

The memory leak when reading this file seems to be specifically related to the prevalence of character columns in this table; not a surprise I guess. Character columns are surprisingly painful to deal with.

8 months later here's a probable fix for this. Unfortunately it's a sort of thing that's hard to write a test for (though maybe one day I will set up a test framework for reference cycle-related issues).

Very subtle!

I tried to find a solution that still avoided unnecessary copying when possible. And I'm sure it can be done, but the FITS_rec and Table code is such a minefield that anything I tried broke 5 other things.

One thing that would alleviate a lot of issues here, that I need to come back to another time, is a Numpy array type that will allow a bytes array to be viewed as a unicode array so long as the characters are decodable in a given encoding (especially ASCII). Currently it doesn't seem to be possible to do that in Numpy without copying as far as I've been able to figure out.

DyND will probably do that someday, when it's ready.

But in the shorter term, a bytes dtype with a declared encoding would be very cool, if it could be pulled off.

I can't keep up with all the stuff Contiuum is doing, but it still kind of irks me that they're doing it while basically drawing away from much new development on Numpy, many of the problems of which I think are fixable with a little love :/

Before I merge this I want to scrounge around for an older issue that reported performance issues with string columns in tables. I want to find that issue and make sure that this fix doesn't introduce a regression on that.

@iguananaut - thanks for getting back to this. I haven't run into it again, so it's apparently in the corners a bit. Anyway, I haven't had a chance to test myself but assuming it plugs the leak for the example I gave then I'm good with this.

Yup, it works for your example. I just need to explore the performance impact of this fix a little more...

In light of this thread (https://groups.google.com/forum/#!topic/astropy-dev/zRIZ6rr4JPg) started by Demitri I'm going to put off merging this fix for now until I can spend a little more time exploring the performance implications of string columns in general. I think that his problem only should have occurred on Python 3 where string copying is guaranteed to occur. If it's happening on Python 2 then I'm at a loss...it's possible there are performance issues unrelated to strings that also need to be considered.

Probably going to shunt this off for a while. I'm working on some new approaches to handling FITS tables that will nullify this issue anyway.

This hasn't actually come up in my daily work since then, even if it isn't fixed.

@embray - is this at all relevant any more? Can I just close this?

I think it's still relevant--it cuts to the core of Numpy's unicode issues.

I just pushed an entirely new fix for this issue (which depends on #4514).

Believe it or not, despite the age of this issue, the root problem was essentially the same (and has actually grown more severe, I think). The issue applies to both Python 2 and 3. This should fix the issue more thoroughly than my original fix, and without any noticeable performance impacts.

One thing I am still slightly mystified by is the role of .copy(). I feel like this issue should not have depended on it, but I could be wrong.

Turns out this is actually far from the end of the reference cycles..... 😭

Okay, I'll probably merge this and open another issue for the other known memory leak. Fixing the other one is probably not worth the effort compared to deprecating the old FITS_rec / ColDefs interface in favor of full replacement with Table.

Oops, I forgot to update the changelog entry again (I saw the changelog entries from #4514 and assumed I had).