Remove compatibility code which forced loading all HDUs on close by saimn · Pull Request #6082 · astropy/astropy

saimn · 2017-05-16T20:05:31Z

Closes #5582, following the discussion there, i.e. Option 2: Remove the compatibility behavior for which almost everybody agreed. To get the old behavior, users can use lazy_load_hdus=False which is equivalent by forcing the reading of all HDUs. So basically this reverts 5c66c2d. And the global option for lazy_load_hdus exists already, so I just added a test to make sure it works.

ping @embray

pllim · 2017-05-16T20:06:52Z

Yay. Still needs a change log.

saimn · 2017-05-16T20:08:58Z

@pllim - yep, I should play the "guess PR number" game ;)

saimn · 2017-05-16T20:11:13Z

astropy/io/fits/tests/test_image.py

        assert phdr['FILENAME'] == 'labq01i3q_rawtag.fits'

-    @raises(ValueError)
+    @raises(IndexError)


The only "unexpected" change is this: with the new behavior the index error when accessing the HDU happens before the ValueError that was raised when trying to access the data. So I think it's fine.

Someone who currently catches that as ValueError will need to change their code, so this needs to be mentioned in your change log.

Good point, done.

Shouldn't we use the contextmanager instead? Doesn't have to be for all tests but since we're changing this one it might be a perfect opportunity.

@saimn , do you plan to address the context manager comment?

@embray - Thanks for reviewing this! You're right about the exception message that should be more helpful, I will implement this. But I'm also confused by your last sentence, is it possible to have _read_all = True if the file is not completely read ?

@embray - self._read_all is set to True when the file is closed (https://github.com/astropy/astropy/blob/master/astropy/io/fits/hdu/hdulist.py#L1146). ~~So it would be necessary to keep the part of the code deleted in this PR which computes self._total_read ?~~
There must be a way to know if the file was fully read (with the EOFError block I guess), but currently it is not possible, so the commit I just pushed modifies the exception message unconditionally for now.

Maybe it's good enough ? I mean is it useful to add more complexity to something already quite complex, just to get a more correct exception message for a very specific case.

I mean, maybe this is actually a bit tricky to determine, but my thinking was that you know the size of the file, and when you read a header you know, at least for a well-formed file, how many bytes should be left in the file (for the data in that HDU). And if there are more bytes beyond the end of the data then you haven't read the whole file yet. There could still be more HDUs.

Yes I agree it could be done, I'm just not sure of the best way. And self._read_all should already give this information, so is it better to add a new way to know if the file was fully read ? Instead why not simply removing the line that set self._read_all to True when the file is closed ? I guess this line is there to avoid running again the beginnning of _read_next_hdu, but if the file is closed it's possible anyway to return earlier.

@embray - I implemented my last suggestion (7381a70), this way _read_all cannot be set to True after the file is closed. Does this work for you ?

pllim · 2017-05-16T20:17:27Z

Guess the PR number games have been successful for me. 😉

bsipocz · 2017-05-16T21:56:50Z

CHANGES.rst

    [#5929]

+  - Remove compatibility code which forced loading all HDUs on close. The old
+    behavior can be used with `lazy_load_hdus=False`. Because of this change,


always use double backticks in the changelog, we never do intersphinx linking from here. Fixing this and the next line will get rid of the docs build error on travis, too.

oops sorry, and yet I know it ...

pllim

Aside of minor comments, LGTM. Also need squashing (and maybe rebase while you are at it)?

pllim · 2017-05-17T17:09:30Z

astropy/io/fits/tests/test_image.py


+    def test_open_3(self):
+        # Test that HDUs cannot be accessed after the file was closed
+        r = fits.open(self.data('test0.fits'))


Should we also test the context manager (since that is how we recommend open be done anyway, or at least I do) or does it not matter?

It does not really matter here as the file is closed just after.

pllim · 2017-05-17T17:10:10Z

CHANGES.rst

+  - Remove compatibility code which forced loading all HDUs on close. The old
+    behavior can be used with ``lazy_load_hdus=False``. Because of this change,
+    trying to access the ``.data`` attribute from an HDU which is not loaded now
+    raises a IndexError instead of a ValueError. [#6082]


Nitpick: Also add double-backticks to IndexError and ValueError.

Ok, I found the two versions in the CHANGES file, so I was not sure which one is preferred.

saimn · 2017-05-17T19:02:08Z

@pllim - Rebased and squashed.

pllim

👍

MSeifert04 · 2017-05-17T21:04:20Z

astropy/io/fits/hdu/hdulist.py

-        # the deprecation period has passed.
-        if self._file and self._file.closed and self._total_read is not None:
-            if index > self._total_read - 1:
-                warnings.warn(textwrap.dedent(


What a shame, we lose the most verbose warning message I've ever seen... 😄

MSeifert04 · 2017-05-17T21:12:24Z

It's a good sign that the tests are passing and the changes look alright. :)

embray · 2017-05-22T15:02:13Z

I have one small-ish comment, but otherwise LGTM thanks @saimn !

bsipocz · 2017-05-23T13:29:14Z

Thank you @embray for reviewing this!

saimn · 2017-05-30T17:11:03Z

Rebased to get rid off a changelog conflict. @embray are you happy with the _read_all solution from 7381a70 ?

saimn · 2017-06-06T22:26:45Z

Not sure if the tests failures are real or related to some pytest change (errors in pytest_runtest_teardown about unclosed files: https://travis-ci.org/astropy/astropy/jobs/237603430#L2494). So rebase to check again with Travis. Also I fixed another ResourceWarning error that was causing an other test to fail.

saimn · 2017-06-07T14:11:55Z

Ok, so the test failures are indeed related to 88315b3 and the --open-files option/plugin. But really I don't understand why, as this commit does not change the behavior [1]. Many fits tests does not close FITS file, before 88315b3 these are not detected by --open-files and now they are.

Example before 88315b3, where --open-files does not fail the test, but there are two ResourceWarning which show that files are not closed:

astropy/io/fits/tests/test_image.py::test_comphdu_bscale PASSED

====================================== warnings summary =======================================
None
  [pytest] section in setup.cfg files is deprecated, use [tool:pytest] instead.

lib.linux-x86_64-3.6/astropy/io/fits/tests/test_image.py::test_comphdu_bscale
  /tmp/simon/astropy-test-j_7zc2ao/lib.linux-x86_64-3.6/astropy/io/fits/tests/test_image.py:1771: ResourceWarning: unclosed file <_io.FileIO name='/tmp/simon/pytest-of-simon/pytest-12/test_comphdu_bscale0/3hdus.fits' mode='rb' closefd=True>
    hdus = fits.open(filename2)
  /home/simon/miniconda3/lib/python3.6/site-packages/_pytest/python.py:142: ResourceWarning: unclosed file <_io.FileIO name='/tmp/simon/pytest-of-simon/pytest-12/test_comphdu_bscale0/3hdus_comp.fits' mode='rb' closefd=True>
    testfunction(**testargs)

-- Docs: http://doc.pytest.org/en/latest/warnings.html
===================================== 69 tests deselected =====================================
===================== 1 passed, 69 deselected, 3 warnings in 1.18 seconds =====================

After 88315b3, --open-files raise an error for these files.
Maybe something because in the first case (before 88315b3) it tries to read past the end of the file (see [1] above, this is the only change I think). It may also depends on how psutil determine the open files.

pllim · 2017-06-07T14:17:01Z

What PR was that commit tied to? My GitHub-fu is insufficient to sniff that out.

bsipocz · 2017-06-07T14:18:56Z

@pllim - To this one, that is the 3rd commit here.

pllim · 2017-06-07T14:20:19Z

Oh... 😅 😅 😅

saimn · 2017-06-07T14:31:51Z

Hehe ! The goal of this commit was to have different messages for the IndexError, if the file was read to the end or not (see #6082 (comment)).

DougBurke · 2017-06-08T14:30:19Z

I'm not sure how related the following is, but I thought I'd mention it here before opening a separate issue. I also apologize for not having time to check against the current master branch, but I only have AstroPy 1.3.3 easily at hand.

In Sherpa we do "interesting" things for some of our file I/O, including trying to read in a file as FITS when it may not be. This ends up creating ResourceWarning warnings in out tests. At least some of these appear to be AstroPy related (i.e. not closing the file up on error) as shown in sherpa/sherpa#365 - which I will repeat the example code here:

% cat dummy.dat 
# A B
0.5 1.65
% 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))
%

Running this script (AstroPy 1.3.3 and Python 3.5) produces the warning:

% 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>

bsipocz · 2017-06-08T14:36:56Z

@DougBurke - I see the same warning on master.

saimn · 2017-06-08T14:44:27Z

@DougBurke - This warning is perfectly normal, as the file is not closed in your example (you should add a finally block to close the file, or use the with statement).

pllim · 2017-06-08T15:02:15Z

Just curious, what if you set memmap=False?

DougBurke · 2017-06-08T15:12:33Z

@pllim (assuming you're talkiing to me; threading is not ideal here!) - I get the same warning if I use the following program:

import warnings
from astropy.io import fits

warnings.simplefilter(action='error', category=ResourceWarning)

try:
    fits.open('dummy.dat', memmap=False)
except OSError as e:
    print("--> {}".format(e))

saimn · 2017-06-08T15:14:56Z

Ooh okay, understood, in this case because of the exception the HDUList object is never returned, and the underlying file pointer cannot be closed. Probably not easy to fix ...

DougBurke · 2017-06-08T16:01:52Z

Is it worth me opening a new ticket for this issue then @saimn ?

pllim · 2017-06-08T16:03:28Z

@DougBurke , yes.

DougBurke · 2017-06-08T16:17:17Z

As GitHub has already noted, I've created #6168 to track this - thanks for the input and sorry for highjacking this PR.

saimn · 2017-06-09T21:42:41Z

So, back to my open files issue (#6082 (comment)): I found why most of the tests do not raise an error with --open-files when they do not close files: https://github.com/astropy/astropy/blob/master/astropy/io/fits/tests/__init__.py#L27
This applies for all tests in classes that inherit from FitsTestCase, and explains why the new failures I see here are with tests in standalone functions. But I still don't understand why the behavior changed for these failing tests ...

Closes astropy#5582. astropy#5065 implemented lazy loading of HDUs in fits files. But it did it in a way that maintains backwards compatibility at the expense of performance, loading the remaining HDUs to allow read the headers after the file was closed. Basically this reverts 5c66c2d

saimn · 2017-06-09T22:18:50Z

I think I found the culprit, the function I added to handle the IndexError was keeping a reference to the HDUList object, preventing the file pointer to be closed by the gc ...

DougBurke · 2017-06-09T22:32:37Z

@saimn does this change mean that my proposed change at #6168 (comment) isn't needed, or are they separate issues?

saimn · 2017-06-09T22:57:12Z

@DougBurke - It's not related, but it makes me think about a workaround for your issue (to avoid the ResourceWarning). Opening the file before passing it to fits.open allows to close it after:

try:
    with open('dummy.dat', 'rb') as f:
        fits.open(f)
except OSError as e:
    print("--> {}".format(e))

saimn · 2017-06-09T23:00:38Z

And tests pass now 🕺 🎉

saimn · 2017-06-12T07:59:40Z

Ok I think it's good to go now !

bsipocz · 2017-06-12T08:08:58Z

There was agreement on this above, so merging this now. Thanks @saimn!

DougBurke · 2017-06-12T22:27:42Z

@saimn - thanks for the suggestion in #6082 (comment) - unfortunately our code isn't written in a way that lets us easily use this approach, but I'll keep it in mind as we refactor.

saimn added the io.fits label May 16, 2017

saimn added this to the v2.0.0 milestone May 16, 2017

saimn commented May 16, 2017

View reviewed changes

saimn force-pushed the fits-close-file branch from 450b078 to 7b9c718 Compare May 16, 2017 20:26

bsipocz reviewed May 16, 2017

View reviewed changes

pllim reviewed May 17, 2017

View reviewed changes

saimn force-pushed the fits-close-file branch from 865a697 to a6e4132 Compare May 17, 2017 18:57

pllim requested a review from MSeifert04 May 17, 2017 19:38

pllim approved these changes May 17, 2017

View reviewed changes

MSeifert04 reviewed May 17, 2017

View reviewed changes

saimn force-pushed the fits-close-file branch from 7381a70 to 35c7432 Compare May 30, 2017 17:08

saimn force-pushed the fits-close-file branch from 35c7432 to 37550b3 Compare June 6, 2017 22:23

saimn mentioned this pull request Jun 7, 2017

Disable pytest-warnings during testing #6163

Merged

DougBurke mentioned this pull request Jun 8, 2017

ResourceWarning from astropy.io.fits.open when input isn't a valid FITS file #6168

Closed

saimn mentioned this pull request Jun 9, 2017

Removing deprecations in preparation of v2.0 #6170

Merged

saimn added 5 commits June 10, 2017 00:12

Make the IndexError for HDUList.__getitem__ more helpful

1f7ef9d

Raise the modified IndexError only if the file was not fully read

27a1a9d

Avoids a ResourceWarning that was catched later in test_invalid_blanks

fa85e71

Fix the reference leak that was preventing GC to close files

60bcb1f

saimn force-pushed the fits-close-file branch from b67916e to 60bcb1f Compare June 9, 2017 22:14

Fix changelog conflict and indentation issue [ci skip]

1e7e382

saimn added the Ready-for-final-review label Jun 12, 2017

bsipocz merged commit 694c1c7 into astropy:master Jun 12, 2017

saimn deleted the fits-close-file branch June 12, 2017 08:10

pllim mentioned this pull request Jun 13, 2017

stpyfits: disable lazy loading spacetelescope/stsci.tools#38

Merged

saimn mentioned this pull request Jun 16, 2017

Remove ResourceWarnings when fits.open is given a non-FITS file (fix #6168) #6177

Merged

pllim mentioned this pull request May 16, 2018

astropy.io.fits documentation on what is available after .close() #7472

Closed

pllim mentioned this pull request Apr 3, 2020

MNT: Undo disable lazy loading hack in stpyfits, maybe spacetelescope/stsci.tools#118

Open

Uh oh!

Conversation

saimn commented May 16, 2017

Uh oh!

pllim commented May 16, 2017

Uh oh!

saimn commented May 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented May 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saimn commented May 17, 2017

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 commented May 17, 2017

Uh oh!

embray commented May 22, 2017

Uh oh!

bsipocz commented May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saimn commented May 30, 2017

Uh oh!

saimn commented Jun 6, 2017

Uh oh!

saimn commented Jun 7, 2017

Uh oh!

pllim commented Jun 7, 2017

Uh oh!

bsipocz commented Jun 7, 2017

Uh oh!

pllim commented Jun 7, 2017

Uh oh!

saimn commented Jun 7, 2017

Uh oh!

DougBurke commented Jun 8, 2017

Uh oh!

bsipocz commented Jun 8, 2017

Uh oh!

saimn commented Jun 8, 2017

Uh oh!

pllim commented May 16, 2017 •

edited

Loading

bsipocz commented May 23, 2017 •

edited

Loading