"output_verify" option forced to "exception" when using fits.open() in a "with" block

[DBerke]

Description

Astropy has different behavior when using the fits.open() command in the following two ways (which I expected to be equivalent):

hdulist = fits.open('file_with_non_compliant_header_card.fits", "update") 
    # make some changes and flush the results with: 
    hdulist.flush(output_verify="warn")
hdulist.close(output_verify="warn")

with fist.open('file_non_compliant_header_card.fits", "update") as hdulist:
    # make some changes and flush the results with:
    hdulist.flush(output_verify="warn")

Expected behavior

In both cases, I expected Astropy to warn me about the non-compliant header card, but not raise an exception (as I don't care about the value of the card in question, and can't do anything about it since it's archival data).

Actual behavior

The expected behavior works in the first code form, but not the second; in the second case, I got a warning as expected from flush(), but then astropy raised an exception (astropy.io.fits.verify.VerifyError) upon exiting the with block. (This also occurs when using the "ignore" option, without the warning obviously.)

I first found this bug in 3.2.3, as I'd been using the file = fits.open...file.close() form with explicit output_verify='warn', and changed it to a with block, when I suddenly started getting the VerifyErrors. The bug persisted in 4.1.dev27006 when I cloned the repo.

Steps to Reproduce

I don't have specific steps to reproduce, but from looking at the code in astropy/io/fits/hud/hdulist.py I think I know why it's happening: the HDUList.close method takes an output_verify argument, but the HDUList.__exit__ method which handles leaving a with block only calls self.close without any arguments. Thus, leaving a with block essentially calls close with its default output_verify="exception" argument, regardless of what the user may have used in the interim. This makes it impossible to use with fits.open... with a less restrictive output_verify option than 'exception'.

I don't think having 'exception' be the default when using a with block is bad, per se (errors should never pass silently, unless explicitly silenced!), it would just be nice to be able to change it. I'm not quite sure how to do so; perhaps having an optional output_verify argument to HDUList.__init__ which could set some sort of instance-spcific self._verify_level attribute, the value of which could be passed by __exit__ to the close method? Leave the default as 'exception' and the current behavior would still be the default, while also allowing the user to set whatever level of verification they want. I could try and take a stab at it myself to see if that would work.

System Details

Darwin-16.7.0-x86_64-i386-64bit
Python 3.7.3 (default, Mar 27 2019, 16:54:48)
[Clang 4.0.1 (tags/RELEASE_401/final)]
Numpy 1.16.4
Scipy 1.3.0
astropy 3.2.3

[pllim]

@DBerke , since you have a patch in mind, if you don't mind, a pull request would keep the conversation going.

cc @saimn and @MSeifert04

[saimn]

I agree that for consistency it would be better to be able to pass output_verify to fits.open. You are on the right track, and it should be quite easy to do since the additional arguments passed to fits.open are already stored in hdulist._open_kwargs, which can be accessed later in __exit__ or close.
One question though is, what should happen when using output_verify to fits.open and calling close, which has a default value for output_verify.

[DBerke]

Thanks, I didn't know about hdulist._open_kwargs—that'll be helpful.

Good question. I think giving fits.close a specific value for output_verify should overwrite a value from fits.open. (Since if you're using close, you really need to use it, whereas the value in open only needs to be used if you aren't using close.) I guess my suggestion would be to give open an output_verify default of "exception", then have close inherit the value from open unless explicitly given a value for output_verify.

It's a bit weird, though, in that close's default technically wouldn't do anything as its value would instead come from open, and has a potential confusing side-effect where you could set an output_verify value in open and have it not appear to do anything because you've set a different one in close. All I can think of to combat that is good documentation on the output_verify kwarg for open ("This value will be overridden by the value given in fits.close"), but I could see that being a potential confusing case for new users.

This is the most backward-compatible solution that comes to mind for me (in that everything will work exactly the same as it has before if you don't know about the new output_verify option for open), but I'm open to suggestions!