Handle valid modes correctly for file objects passed to fits.append #7856

drdavella · 2018-10-01T19:48:57Z

This fixes #7854.

astropy-bot · 2018-10-01T19:49:00Z

Hi there @drdavella 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

drdavella · 2018-10-01T19:50:54Z

@pllim I'm not sure whether this one is actually a backport.

pllim · 2018-10-01T19:51:00Z

astropy/io/fits/tests/test_convenience.py

+
+        append_file = tmpdir.join('append.fits')
+        with append_file.open(mode) as handle:
+            fits.append(filename=handle, data=np.random.random((4, 4)))


Do we really need random generator? np.zeros((4, 4)) would suffice?

pllim · 2018-10-01T19:52:38Z

@drdavella , feel free to re-milestone. Usually bug fix is backported, but if backport is unnecessary or impossible for this one, we don't have to. Still need a change log regardless?

bsipocz · 2018-10-01T21:08:49Z

I wonder whether this also fixes #7435 or #7850 is still necessary?

drdavella · 2018-10-02T14:07:56Z

@bsipocz that appears to be an unrelated issue, so #7850 is still necessary.

drdavella · 2018-10-02T14:10:01Z

@at88mph if you have a chance can you please test this to make sure it fixes the issue you reported?

at88mph · 2018-10-02T20:30:01Z

@drdavella Yes, this fixes my issue. Many thanks.

saimn · 2018-10-02T21:38:48Z

astropy/io/fits/hdu/hdulist.py

-        fileobj = _File(fileobj, mode='ostream', overwrite=overwrite)
+        # This can accept an open file object that's open to write only, or in
+        # append/update modes but only if the file doesn't exist.
+        fileobj = _File(fileobj, mode=mode, overwrite=overwrite)


@drdavella - Do we really need a mode keyword in addition to the fileobj's mode ? I don't think that we want this, as it would be really confusing. And the goal of the FITS mode / file mode mapping is to avoid this.
The deleted comment above says that ostream should work also for "append/update modes but only if the file doesn't exist", we could maybe find why it is not the case ?

The issue comes from the check objmode = _normalize_fits_mode(fileobj_mode(fileobj)), which brings us back to 973f94b ;)
We could maybe just add a special case for this ?

Do we really need a mode keyword in addition to the fileobj's mode ? I don't think that we want this, as it would be really confusing.

This is the situation that we have already with _File and fits.open, and we're stuck with those. Unfortunately, I think that adding mode is the right course of action here, because the previous behavior of assuming mode='ostream' for all uses of writeto is incorrect. Luckily, most users never have to think about this since the default of ostream seems reasonable for most cases. But #7854 demonstrates that the assumption is not correct for all cases.

As for _normalize_fits_mode and the commit/PR you referenced: the point of those changes was that the mode checks were previously not being performed correctly in all cases. Remember, we already let users pass mode= to _File and fits.open even though they can pass a file object with its own mode. So we have to perform the compatibility check. I believe we are now performing mode checks correctly. That PR exposed some incompatible uses in the FITS code base. #7854 is another such case that was missed because there wasn't a test case. If we add some sort of special case here, I'm not sure we can guarantee that we will continue to check the mode compatibility correctly in all other cases.

However, I'm open to suggestions if you have a particular idea of what the special case would look like.

Btw, I agree that this is quite confusing, both for users and developers. I'm not quite sure why FITS needs to make reference to modes like ostream, append, etc. at all. Why can't we just be consistent with the Python API? There's probably a good reason, but in any case changing this would require a complete overhaul of the io.fits API, which is probably a non-starter 😉.

Keep in mind that astropy.io.fits was PyFITS, and PyFITS existed way way back when Python couldn't do some of the stuff it can do now. 😉

It works with 2.0 (before _normalize_fits_mode):

In [1]: %astropy Numpy 1.15.2 Astropy 2.0.8 In [2]: with open('foo.fits', mode='ab+') as f: ...: fits.append(f, data=np.zeros(4)) In [3]: fits.info('foo.fits') Filename: foo.fits No. Name Ver Type Cards Dimensions Format 0 PRIMARY 1 PrimaryHDU 5 (4,) float64

So doing a less stringent test should work just as before without adding new bugs or side effects imo. If you prefer adding the mode guessing as in fits.append, I'm also fine with it, but I think that we should avoid adding a new keyword, even if it is hidden.

It's not at all clear to me how to handle this particular case without performing a less stringent test in all cases. I would argue that downgrading this check means that we don't actually care whether we conform to the API as described. The consequence will be that the behavior of the mode argument to fits.open is sometimes unpredictable and possibly just plain wrong.

Keep in mind that the changes associated with _normalize_fits_mode were not unmotivated: the API was actually incorrect prior to that change. So we're saying we're okay with reverting to that state, and we will need to remove and/or modify some associated unit tests.

BTW, I realize I must seem quite stubborn to you, but I think it boils down to this: you appear to resist introducing complexity into the API, which I definitely respect, but I believe in this case it would mean sacrificing correctness and consistency in other parts of the API.

@drdavella - I know this was added for good reasons, and I understand your concerns - except for the emphasis about the API. For me it is mostly an internal thing, that users should not care about. My concern is that in terms of "public" API, something like this was working (in 2.0):

with open('foo.fits', mode='ab+') as f: hdu = fits.ImageHDU(data=np.ones(5)) hdu.writeto(f)

and now fails, and with the change that you propose here, users would have to use this:

with open('foo.fits', mode='ab+') as f: hdu = fits.ImageHDU(data=np.ones(5)) hdu.writeto(f, mode="append")

or this

with open('foo.fits', mode='wb+') as f: hdu = fits.ImageHDU(data=np.ones(5)) hdu.writeto(f, mode="update")

This is a weird API for me ;). So I think that we can find a way to manage this internally, even if this needs some sacrificen, it will not be the first hack in the io.fits code. (And I would be happy if we had a better solution but I don't see one currently...)

Okay yes, I agree that those cases are definitely not desirable. My hope/intention was that the mode argument to *HDU.writeto would be completely unnecessary and invisible to users for most or all use cases, but that's apparently not the case.

I'll add these examples as test cases (always good to have more of those!) and will try to work out a better solution here.

bsipocz · 2018-10-12T00:50:21Z

I'm trying to push 2.0.9 out of the door today, so remilestoning this for the next one.

bsipocz · 2018-11-02T00:59:43Z

@drdavella @saimn - What's the status of this? 2.0.10 will be part of 3.1, so would be nice to get all affected PRs to the release candidate if possible :)

drdavella · 2018-11-02T14:08:44Z

As long as @saimn approves, I think this is ready to go.

saimn

Changelog entry must be moved to v2.0.10 (and probably needs a rebase)

saimn · 2018-11-05T11:52:48Z

@drdavella - I did not see the new commits, sorry. This looks good now, thanks!

drdavella · 2018-11-05T13:01:47Z

@saimn done: CHANGES.rst

saimn · 2018-11-05T13:52:02Z

Thanks @drdavella !

Handle valid modes correctly for file objects passed to fits.append

drdavella requested a review from saimn October 1, 2018 19:49

drdavella added the Bug label Oct 1, 2018

pllim added the io.fits label Oct 1, 2018

pllim added this to the v2.0.9 milestone Oct 1, 2018

pllim reviewed Oct 1, 2018

View reviewed changes

drdavella force-pushed the fits-append-mode branch from 44a518a to 3dd9b77 Compare October 1, 2018 19:51

saimn reviewed Oct 2, 2018

View reviewed changes

drdavella force-pushed the fits-append-mode branch from d9b2ff3 to eae1c31 Compare October 9, 2018 16:27

drdavella mentioned this pull request Oct 11, 2018

fits.append() does not support documented fits.open() kwargs #7882

Closed

bsipocz modified the milestones: v2.0.9, v2.0.10 Oct 12, 2018

saimn requested changes Nov 5, 2018

View reviewed changes

drdavella added 4 commits November 5, 2018 07:58

fits.append now handles modes correctly for file objects

5129660

Update change log

b7d8696

Add test for HDU writeto using various file modes

b9a22f5

Better solution for automatic selection of fileobj modes

a17d4e6

drdavella force-pushed the fits-append-mode branch from eae1c31 to a17d4e6 Compare November 5, 2018 13:00

Remove unwanted changelog entries [skip ci]

38caf82

saimn approved these changes Nov 5, 2018

View reviewed changes

saimn merged commit 0725a5c into astropy:master Nov 5, 2018

drdavella deleted the fits-append-mode branch November 5, 2018 14:27

bsipocz pushed a commit that referenced this pull request Nov 8, 2018

Merge pull request #7856 from drdavella/fits-append-mode

f3d7149

Handle valid modes correctly for file objects passed to fits.append

bsipocz pushed a commit that referenced this pull request Nov 8, 2018

Merge pull request #7856 from drdavella/fits-append-mode

6eeac69

Handle valid modes correctly for file objects passed to fits.append

bsipocz pushed a commit that referenced this pull request Nov 9, 2018

Merge pull request #7856 from drdavella/fits-append-mode

62bdd85

Handle valid modes correctly for file objects passed to fits.append

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle valid modes correctly for file objects passed to fits.append #7856

Handle valid modes correctly for file objects passed to fits.append #7856

drdavella commented Oct 1, 2018

astropy-bot bot commented Oct 1, 2018 •

edited

Loading

drdavella commented Oct 1, 2018

pllim Oct 1, 2018

pllim commented Oct 1, 2018

bsipocz commented Oct 1, 2018

drdavella commented Oct 2, 2018

drdavella commented Oct 2, 2018

at88mph commented Oct 2, 2018

saimn Oct 2, 2018

saimn Oct 2, 2018

drdavella Oct 3, 2018 •

edited

Loading

drdavella Oct 3, 2018 •

edited

Loading

pllim Oct 3, 2018

saimn Oct 5, 2018

drdavella Oct 8, 2018 •

edited

Loading

drdavella Oct 8, 2018

saimn Oct 8, 2018

drdavella Oct 9, 2018

bsipocz commented Oct 12, 2018

bsipocz commented Nov 2, 2018

drdavella commented Nov 2, 2018

saimn left a comment

saimn commented Nov 5, 2018

drdavella commented Nov 5, 2018

saimn commented Nov 5, 2018

Handle valid modes correctly for file objects passed to fits.append #7856

Handle valid modes correctly for file objects passed to fits.append #7856

Conversation

drdavella commented Oct 1, 2018

astropy-bot bot commented Oct 1, 2018 • edited Loading

drdavella commented Oct 1, 2018

Choose a reason for hiding this comment

pllim commented Oct 1, 2018

bsipocz commented Oct 1, 2018

drdavella commented Oct 2, 2018

drdavella commented Oct 2, 2018

at88mph commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drdavella Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

drdavella Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drdavella Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsipocz commented Oct 12, 2018

bsipocz commented Nov 2, 2018

drdavella commented Nov 2, 2018

saimn left a comment

Choose a reason for hiding this comment

saimn commented Nov 5, 2018

drdavella commented Nov 5, 2018

saimn commented Nov 5, 2018

astropy-bot bot commented Oct 1, 2018 •

edited

Loading

drdavella Oct 3, 2018 •

edited

Loading

drdavella Oct 3, 2018 •

edited

Loading

drdavella Oct 8, 2018 •

edited

Loading