Ensure FITS filestream is closed on generic read

[bmorris3]

The generic light curve reader opens FITS files to determine how to parse the contents. The call to fits.open leaves a file stream open, which can prevent later and/or downstream calls from opening that same file. This workaround ensures the filestream is closed, and simply makes a copy of the list hdulist.

[bmorris3]

With further testing, it looks like this particular failure may be the result of a mismatch between the astropy and lightkurve versions. If I update to astropy dev, no fix is needed. I'll keep investigating.

[Nschanche]

This may not work as a change. I think the data stream needs to remain open as we still need access to the data.
For example, line 49 calls Table.read(hdulist[ext], format="fits"). With the proposed change, this fails with the error, "ValueError: I/O operation on closed file".

[christinahedges]

@Nschanche @bmorris3 there are parts of the LightCurve and TargetPixelFile classes that rely on these files being open, as @Nschanche points out.

We could change the functionality so that the hdu attribute always reopens the file, rather than leaving it open. @Nschanche what do you think?

[bmorris3]

I was wrong that my implementation was making a copy of the hdulist, which I've corrected in the latest commit (e50c2e2).

I see the hdu attribute challenge in the LC/TPF classes. I'll make another commit in a minute with a similar fix there. It doesn't look like an open file stream is ever needed, since lightkurve doesn't modify the original file on disk, only makes copies.

[bmorris3]

I think in ab001e7 I've closed the remaining open file streams. If #1299 (comment) isn't correct and you do need open file streams anywhere, I'm happy to revise again.

[Nschanche]

Yes, I agree they don't need to be open, the data just needs to be accessible. The deep copy you added looks good to me.

[bmorris3]

Thanks @Nschanche! Let me know if there's anything else you'd like added @christinahedges.

[christinahedges]

Hey @bmorris3 I am a little confused on this PR and I want to be sure I understand, can you post a comment here with an example showing what the issue is you're encountering, and why you're suggesting this is a fix?

[dfm]

I'm finding that this PR introduces one regression (something to do with pickling) that I'll try to track down, but otherwise I think this should be merged to catch memory leaks!

Edit: I'd say that it could be better to refactor such that we don't need to carry around a full copy of the HDUList (which should contain all the data in memory), but that's a much larger refactor than what is suggested here.

[bmorris3]

@christinahedges I opened this PR because if you read a local FITS file with lightkurve, it keeps the file stream open. If other packages/scripts read that same FITS file after lightkurve has opened but not closed the file stream, they can crash.

This exact problem was occurring specifically in spacetelescope/lcviz#10, which caused two tests to fail. We opted to merge that PR after commenting out the failing tests, with the plan to uncomment them when this lightkurve PR is merged.

[bmorris3]

@christinahedges Does that sound ok?

[christinahedges]

This looks good to me, I think @dfm is right that we need to eventually have a better solution for carrying around this fits file.

[orionlee]

@bmorris3 What are the use cases that leaving the file stream open become a problem?

My understanding is that normally, once a LightCurve object is longer in use, the file stream will be closed. Simultaneous read from multiple clients on the same underlying file is not a problem (at least on Windows / Linux).

The workaround here, with deepcopy(hdulist), causes memory leak in typical (if not all) cases. See #1388 (comment).

---

One scenario that leaving the file stream open would cause problem is in opening corrupted FITS file on Windows platform. For that specific case, another solution can be used (ensure the generic reader close hdulist upon any exception during read).

[bmorris3]

@orionlee The problematic case is explained here: #1299 (comment).

[orionlee]

@bmorris3 Just to be sure, are you referring to the test failures in #1299 (comment) , specifically, the ResourceWarning about unclosed file? E.g.

78: PytestUnraisableExceptionWarning: Exception ignored in: <_io.FileIO [closed]>

  Traceback (most recent call last):
    File "C:\dev\lightkurve\src\lightkurve\io\kepler.py", line 34, in read_kepler_lightcurve     
      lc = read_generic_lightcurve(
  ResourceWarning: unclosed file <_io.BufferedReader name='C:\\Users\\SL\\.astropy\\cache\\download\\url\\0dd9ac829437a6265e20d71bc15c9e18\\contents'>