Remove ResourceWarnings when fits.open is given a non-FITS file (fix #6168) by DougBurke · Pull Request #6177 · astropy/astropy

DougBurke · 2017-06-10T20:29:40Z

This is not a complete fix - it's missing a changelog entry, could be squashed down into a single commit - but has been made to see if this change works - as asked for by @saimn at #6168 (comment)

The two main concerns I have - given that I don't know the code base - are:

Is the place I chose to insert the close call the best place for it? I started at the suggestion of ResourceWarning from astropy.io.fits.open when input isn't a valid FITS file #6168 (comment) and then looked up the calling chain a bit
Do I only need to close the file handle, or are there other objects that need closing?

Edit 2017/06/18 I now believe this to be ready for review for inclusion. There are open questions about whether the location of the close is appropriate and whether it should be extended to other file types (mainly the compressed files). For the latter point I claim the current code is an improvement, and could be applied before working out what to do with the other file types, but I'm biased as this PR fixes my needs for now.

Edit 2017/06/20 as far as I'm concerned I've addressed all the comments (and have squashed the PR to a more-comprehensible commit).

bsipocz · 2017-06-12T00:03:46Z

+                        # Close the file: see
+                        # https://github.com/astropy/astropy/issues/6168
+                        #
+                        # TODO: is this a sensible place to close things?


I would remove these question like todos from the code, and discuss it in the PR itself

MSeifert04 · 2017-06-12T16:02:08Z

+            f.write('0.1 0.2\n')
+
+        # This should raise an IOError because there is no end card.
+        with catch_warnings() as ws:


That seems a bit fragile without providing the warning-type explicitly. I don't know where RessourceWarning is defined but we should definetly only catch these here.

It's a very good question as to why I didn't restrict catch_warnings to just ResourceWarning. I blame it on exploratory code. When I get time I'll fix this.

It would be good to check that the test fails without your fix: ResourceWarning is ignored by default (https://docs.python.org/3/library/warnings.html#default-warning-filters), not sure what's the status with pytest's warning plugin.

Also I don't think you can tell which warnings you want to catch with catch_warnings(). If needed the list of caught warnings can be filtered, but here the goal is to have no warning so the test seems fine.

@saimn What you mean with "I don't think you can tell which warnings you want to catch", it should be possible according to the docs: http://docs.astropy.org/en/stable/api/astropy.tests.helper.catch_warnings.html#catch-warnings

@MSeifert04 - Oh sorry, I thought it was using warnings.catch_warnings, I didn't know (or remember) the Astropy wrapper.

MSeifert04 · 2017-06-12T16:02:34Z

+
        # If we're trying to read only and no header units were found,
-        # raise and exception
+        # raise an exception


😄 good catch!

MSeifert04 · 2017-06-12T16:04:27Z

I left some comments but I don't know if that's the correct place either. I'm also not sure if I have time to look into this the next days.

saimn · 2017-06-13T11:21:41Z

Thinking about this, I'm surprised there is no other way currently to check if a file is a FITS one or not. There is actually some code to identify a FITS file and it's used for the Table reader. It simply checks that the file starts with the SIMPLE = T line, not sure if this could also be used in fits.open ?

So about where to insert the close, the _read_next_hdu is already quite complicated, with 3 try/except levels, I would prefer not adding another except here. Hence my suggestion which was to put it in HDUList._readfrom when it tries to read the PrimaryHDU.
Also if a file object is given to fits.open, then it must not be closed. I think this can be checked with HDUList._file.file_like.

DougBurke · 2017-06-13T11:38:28Z

The reason why I put it in _read_next_hdu is because I was being defensive, in that I was imagining the (unlikely) case where the first header can be read in but then there's a problem reading in one of the other headers. Although as I don't understand the data flow here I may not have a good grasp on what is going on.

saimn · 2017-06-13T12:35:14Z

Yeah I understand your approach, and have mixed feelings on this. I agree the file should also be closed if it fails on the next HDUs, though it's unlikely to happen.

Something like this was suggested in the PR review - see astropy#6177 (comment) - and it has uncovered an apparent flaw in my fix, as the new test that was added to check this fails (it passes if the changes to hdulist.py are reverted).

DougBurke · 2017-06-13T13:03:39Z

@saimn I tried to add a check to not close the file if it's actually a file-like object, but the test I added to check this fails. As this new test passes without my changes to hdulist.py, I think one or more of:

calling fileobj.close() is wrong
my understanding of how to use HDUList._file.file_like is wrong

I didn't try moving the location of the close as I didn't want to change too many things at once.

saimn · 2017-06-13T13:33:38Z

@DougBurke - My bad, file_like is a different thing, ("file-like object, i.e. one that has read and/or write methods"). So I don't know if there is a way to know if the input file was a file object or a filename.

DougBurke · 2017-06-13T13:39:14Z

@saimn - well, I think the overall point you brought up - that file objects shouldn't be closed - is a good one, so it's not all bad ;-)

saimn · 2017-06-14T10:08:39Z

                    except IOError:
+                        # Close the file: see
+                        # https://github.com/astropy/astropy/issues/6168
+                        fileobj.close()


Use self._file.close() instead (it takes care of the memap).

saimn · 2017-06-14T10:11:29Z

@DougBurke - Right ;). So about closing the file only if a filename was passed, I think you need to add a flag in self._file._open_filename (in a way similar to file_like).

DougBurke · 2017-06-14T13:35:39Z

@saimn - here's my initial attempt at adding in a flag as you suggested. I'll include my commit message for 2c45952 as it's not clear to me whether my bodge is sufficient:

This is intended to allow the HDUList object to know whether the file
should be closed on error or not. At present it is only applied to
simple file reads, but it seems like it could also be used for the
path where an intermediate (temporary) file is used (e.g. the
compressed-file options), although perhaps the fact that these tend
to be using a tempfile.NamedTemporaryFile object means they get
cleaned up properly anyway.

saimn · 2017-06-16T08:15:23Z

Could you rebase on master to make sure this works with #6082 ?

DougBurke · 2017-06-16T13:03:42Z

Rebasing. The original tip was 2c45952

The reason for not closing the bzip/gzip handlers was mentioned in the commit message and comment at #6177 (comment) (well, it's hardly a reason why, more saying I wasn't sure if it was a sensible thing to do, since I don't want to create new warnings due to closing stuff that's going to close anyway). I'd feel safer having tests to check these cases, but don't know if I have the time or knowledge to create them.

On my machine the test TestWebProfile.test_main failed which I am going to assume is nothing to do with my changes.

Something like this was suggested in the PR review - see astropy#6177 (comment) - and it has uncovered an apparent flaw in my fix, as the new test that was added to check this fails (it passes if the changes to hdulist.py are reverted).

saimn · 2017-06-16T21:35:38Z

Oh sorry, it seems I did not read carefully your commit message 😕 . So now I understand why, but I think these objects should also be closed (though I agree it's difficult to do without testing).

saimn · 2017-06-16T21:45:34Z

So, the feature freeze for v2.0 is very close, and given my timezone it's difficult for me to help further. I think it's almost ready, not too risky because it's just closing the file in case of an exception. But it would be great to have another opinion, @pllim @MSeifert04 ?

DougBurke · 2017-06-16T21:56:41Z

Well, I'm only available to work on this early morning or late night here, so my timezone is shot! Thanks for the input @saimn

DougBurke · 2017-06-18T20:06:30Z

I've added an entry to CHANGES.rst and this PR meets my needs, so I'm ready for review. Let me know if you'd like this squashed to remove some of the exploratory changes made in the PR.

saimn · 2017-06-19T09:24:32Z

Thanks @DougBurke, I tested with gzip and bz2 but they do not raise the ResourceWarning (maybe because the file is opened at the C level).
Another point though, while looking at the code, is the ignore_missing_end parameter. With this the exception is not the same (it goes though the except EOFError block, and then raised as IOError in _readfrom). So in this case the file is not closed, and the same kind of fix could be applied. Would you have time to address this case ?

Otherwise this looks good so no objection from me to get this in 2.0. Also, if you can squash the commit it's great.

DougBurke · 2017-06-19T13:06:05Z

@saimn where you thinking of something like c4f1dc6?

saimn · 2017-06-19T13:20:42Z

@DougBurke - Yes, looks good ! Just left 2 minor comments, otherwise I think we can merge once the tests pass.

DougBurke · 2017-06-19T13:38:30Z

@saimn - if 76927ae passes then tonight (US East coast) I'll try to squash the commits. I'm guessing just one commit for this PR is needed?

saimn · 2017-06-19T14:19:26Z

@DougBurke - There is no strict policy about squashing commits, but here, given the small number of changed lines it would make the history cleaner (it's appreciable when you use git blame for instance).
Tests are passing (except an unrelated timeout error) so just waiting for the squashing ;)

pllim · 2017-06-19T14:22:42Z

Hold your horses, I am reviewing...

pllim

I need some answers before I am comfortable with this being merged. Thanks for your patience!

pllim · 2017-06-19T14:15:54Z

  - Use more sensible fix values for invalid NAXISj header values. [#5935]

+  - Close file on error to avoid creating a ``ResourceWarning`` warning
+    about an unclosed file. [#6168]


You also need to include this PR's number: [#6168, #6177]

Good catch :)

pllim · 2017-06-19T14:21:16Z

+
+    # Avoid a spurious pyflakes warning about an unused import
+    # of six by adding a reason string
+    @pytest.mark.skipif(six.PY2, reason='six.PY2')


To avoid pyflakes warning, you could have just write from ....extern import six # noqa above. The reason here should be used to explain why the test is skipped for Python 2 (although I know you explained it in the docstring).

But I am okay with leaving this as-is because Python 2 support will be removed in about 3 months anyway.

pllim · 2017-06-19T14:23:37Z

+    # Avoid a spurious pyflakes warning about an unused import
+    # of six by adding a reason string
+    @pytest.mark.skipif(six.PY2, reason='six.PY2')
+    @pytest.mark.parametrize('ignore', [False, True])


Why does this need to be two different parametrize decorators? Just one would be sufficient as it can support multiple input arguments at once.

To me it's clearer that it's iterating through both the ignore and fileobj arguments (i.e. you get 2 * 2 combinations of tests). If I write

@pytest.mark.parametrize('ignore,fileobj', [(False, False), (False, True), (True, False), (True,True)])

it is harder for me to check that I have all combinations.

pllim · 2017-06-19T14:25:18Z

+        open a non-FITS file when using a filename.
+        """
+
+        filename = self.temp('not-fits.fits')


The original issue mentioned *.dat file but here you are testing with *.fits. Probably does not matter (if FITS code does not impose extension hard-coding) but I would like to know if this test actually fails in current master without this PR.

The code does not use the extension afaik.

@pllim - in one of my original commits I wrote a test - using the file name 'not-fits.fits' - and made sure that it failed before I changed any code. You can see this at

https://github.com/DougBurke/astropy/blob/693a032cd0e5a13c4e10b8c6f60907252bedf8f3/astropy/io/fits/tests/test_hdulist.py#L918

It was not tested against the current master branch, but whatever was master at the time.

pllim · 2017-06-19T14:35:22Z

+    # of six by adding a reason string
+    @pytest.mark.skipif(six.PY2, reason='six.PY2')
+    @pytest.mark.parametrize('ignore', [False, True])
+    @pytest.mark.parametrize('fileobj', [False, True])


I actually don't understand this part. I skimmed through the conversation but none of it explains to me why fileobj needs to be a test input. Shouldn't you be checking to see if filename is a file object instead of an external input like this? Something like if not isinstance(filename, six.string_types): ... # a file object. Of course, then you have to first create the file object at the start of the test. Am I missing something here?

Oh actually in my previous comment I did not mean to parametrize fileobj, but just to put the two with blocks in the same test, to test both with ignore_missing_end. Here it gives the same result but it generates the file four times.

Response to @saimn 's comment:

Well, I realized that I really should check that the ignore_missing_end behavior is checked when a user has explicitly opened the file. Given the use of pytest.mark.parametrize I realized that I could get all versions written in a single - but ugly - test.

Repsonse to @pllim comment

I originally had two tests, one where the file was passed as a filename and one where it was passed as a file object. To avoid some code repetition I combined them into a single test, and used a boolean flag (the fileobj parameter) to determine what was to be done. This was partly in response to my mis-understanding of a comment by @saimn

Doing a file I/O 4x when 2x is sufficient is undesirable. I recommend you and @saimn sort out the misunderstanding before merge. Thanks!

Just to note: the file sizes in question are very small, so it's not a large amount of I/O that is being duplicated.

DougBurke · 2017-06-19T19:53:40Z

I have unified the tests so that the file is only created once, and then the four different tests are run on it. if this passes I'll promise I'll get to squash it.

saimn · 2017-06-19T20:34:39Z

In the end the same combination of parameters were tested I think, it's just that there are several way to test all combinations. But I like the latest version without parametrize, it's simpler and easier to understand so 👍
Most tests already passed, if you want to rebase.
@pllim - Good for you ?

Python 3 warns about open file handles on exit (via a ResourceWarning warning) which fits.open can create if given an invalid FITS file. This commit causes the opened file to be closed on error and adds a test to check that the warnings are no-longer raised. Care needs to be taken to not close a file handle if it was actually sent in, which required adding an extra bit of state to the FITS object (the close_on_error flag). It appears that compressed files do not cause this warning (and so no close is needed there).

DougBurke · 2017-06-19T20:57:30Z

Pushing a hopefully rebased and squashed PR. The last commit before this was 5848f80 .

DougBurke · 2017-06-20T12:39:53Z

The three errors from the Travis tests do not appear to be related to this PR since they are:

a timeout in the test_online_sts test: https://travis-ci.org/astropy/astropy/jobs/244689991
two test runs that were cancelled as they exceeded the time limit: https://travis-ci.org/astropy/astropy/jobs/244689995 and https://travis-ci.org/astropy/astropy/jobs/244689996

saimn · 2017-06-20T12:55:09Z

Yeah errors are not related, it seems the astropy server is not in good shape.

pllim · 2017-06-20T13:53:33Z

LGTM and I agree that failures are unrelated. Merging. Thank you!

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

DougBurke changed the title ~~Bugfix for issue 6188~~ Bugfix for issue 6168 Jun 10, 2017

DougBurke mentioned this pull request Jun 10, 2017

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

Closed

bsipocz reviewed Jun 12, 2017

View reviewed changes

bsipocz added the io.fits label Jun 12, 2017

DougBurke changed the title ~~Bugfix for issue 6168~~ Remove ResourceWarnings when fits.open is given a non-FITS file (fix #6168) Jun 12, 2017

pllim added the Work in progress label Jun 12, 2017

pllim requested review from MSeifert04, embray and saimn June 12, 2017 14:28

MSeifert04 reviewed Jun 12, 2017

View reviewed changes

saimn reviewed Jun 14, 2017

View reviewed changes

DougBurke force-pushed the bugfix-for-issue-6188 branch from 2c45952 to 07ecc34 Compare June 16, 2017 13:04

bsipocz added the Ready-for-final-review label Jun 18, 2017

bsipocz removed the Work in progress label Jun 18, 2017

bsipocz added this to the v2.0.0 milestone Jun 18, 2017

saimn approved these changes Jun 19, 2017

View reviewed changes

pllim reviewed Jun 19, 2017

View reviewed changes

DougBurke force-pushed the bugfix-for-issue-6188 branch from 5848f80 to 86bf020 Compare June 19, 2017 20:57

pllim merged commit 033e9cb into astropy:master Jun 20, 2017

bsipocz pushed a commit that referenced this pull request Jun 20, 2017

Merge pull request #6177 from DougBurke/bugfix-for-issue-6188

40ca691

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

DougBurke deleted the bugfix-for-issue-6188 branch June 20, 2017 16:21

Conversation

DougBurke commented Jun 10, 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!

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!

MSeifert04 commented Jun 12, 2017

Uh oh!

saimn commented Jun 13, 2017

Uh oh!

DougBurke commented Jun 13, 2017

Uh oh!

saimn commented Jun 13, 2017

Uh oh!

DougBurke commented Jun 13, 2017

Uh oh!

saimn commented Jun 13, 2017

Uh oh!

DougBurke commented Jun 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saimn commented Jun 14, 2017

Uh oh!

DougBurke commented Jun 14, 2017

Uh oh!

saimn commented Jun 16, 2017

Uh oh!

DougBurke commented Jun 16, 2017

Uh oh!

saimn commented Jun 16, 2017

Uh oh!

saimn commented Jun 16, 2017

Uh oh!

DougBurke commented Jun 16, 2017

Uh oh!

DougBurke commented Jun 18, 2017

Uh oh!

saimn commented Jun 19, 2017

Uh oh!

DougBurke commented Jun 19, 2017

Uh oh!

saimn commented Jun 19, 2017

Uh oh!

DougBurke commented Jun 19, 2017

Uh oh!

saimn commented Jun 19, 2017

Uh oh!

pllim commented Jun 19, 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!

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

DougBurke commented Jun 10, 2017 •

edited

Loading