Repository navigation
ResourceWarning from astropy.io.fits.open when input isn't a valid FITS file #6168
Description
With astropy 1.3.3 and python 3.5 (and apparently also the current master branch), the following code
% cat dummy.py
import warnings
from astropy.io import fits
warnings.simplefilter(action='error', category=ResourceWarning)
try:
fits.open('dummy.dat')
except OSError as e:
print("--> {}".format(e))
produces this output (the problem is the ResourceWarning that is being raised):
% python dummy.py
--> Header missing END card.
Exception ignored in: <_io.FileIO name='dummy.dat' mode='rb' closefd=True>
ResourceWarning: unclosed file <_io.FileIO name='dummy.dat' mode='rb' closefd=True>
for an invalid FITS file, such as
% cat dummy.dat
# A B
0.5 1.65
There has been some discussion of this at #6082 (comment) but it seems worth a separate issue.
Note that this may seem an odd thing to do - i.e. use a FITS open command on a non-FITS file - but it would be useful to https://github.com/sherpa/sherpa to handle this without the warning (since we do some odd things in the name of not breaking user expectations). At present we have to skip these warnings in our tests and it would be preferable to have warning-free code as much as possible.
Activity
pllim commentedon Jun 8, 2017
Might be somehow related to #4795 ? Just for the record,
fits.getheader(),fits.getdata(), andTable.read(..., format='fits')also produce this.Why not use
Table.read(...)and let it auto-determine your file type? I know that does not fix the underlying FITS problem but at least you don't have to deal with it on your end.DougBurke commentedon Jun 8, 2017
Well, it would be one reason for us to finally get rid of our support for pyfits! This is happening in low-level code in our application, so I'm reluctant to make large-scale changes like using
Tablewithout beefing up our tests at this time, but it may well be a sensible direction to head in. Thanks for the suggestion.DougBurke commentedon Jun 8, 2017
Ahh, looks like @olaurino already reported this issue during the discussion of #2824 (so you can consider this the issue Omar talked about opening ;-)
saimn commentedon Jun 8, 2017
Nice find for #2824 ! The fix is not difficult in itself, the question is to know where is to the good place to close the file. Probably in
HDUList._readfrom, when trying to read the first HDU: https://github.com/astropy/astropy/blob/master/astropy/io/fits/hdu/hdulist.py#L1043Catching the IOError here to close the file should do the job. If someone wants to try and test 😇
DougBurke commentedon Jun 9, 2017
Before trying to fix the problem I wanted a test to check I could reproduce it. Does this test seem sensible / meet AstroPy standards?
https://github.com/DougBurke/astropy/blob/b3de1e3fdce0e071f64ab99b1d625b0be6f4fe65/astropy/io/fits/tests/test_hdulist.py#L873
saimn commentedon Jun 9, 2017
@DougBurke - Yes, looks good.
DougBurke commentedon Jun 9, 2017
I have an initial fix which makes the test pass on my machine, but I'm not 100% convinced that it is a sensible fix (given that there's a lot of moving parts here). Can someone look at
DougBurke@3eb9683
and see if it is sensible? As noted in the comments, my two main concerns are
fileobjor should other things be closed too (or a different abstraction used to closefileobj)?saimn commentedon Jun 9, 2017
Can you open a PR with this ? I don't know if it is the good fix but it would be easier to discuss with the code, and it would allow to run the CI with your fix.
DougBurke commentedon Jun 9, 2017
DougBurke commentedon Jun 10, 2017
See #6177
Close files on fits.open error (fix astropy#6168)
Merge pull request #6177 from DougBurke/bugfix-for-issue-6188
Merge pull request #6177 from DougBurke/bugfix-for-issue-6188