current workaround #4792 for appveyor python 3.5 does not work correctly by MSeifert04 · Pull Request #4793 · astropy/astropy

MSeifert04 · 2016-04-20T08:15:25Z

I thought #4792 might be a good idea and at least the tests are passing but inside this workaround just hides the error message. So I dug a bit deeper and I found out that the function io.fits.util._is_append_mode_platform was just a workaround because python.io.FitsIO-Instances lost their "a"-mode. This has been fixed for a while and ported to all supported python versions 2.7 and 3.3+

Then I tried some things one could put into io.fits.open and noticed that on my machine the function call was unnecessary for all things I could think of (also including some things I wouldn't have thought of).

So this first commit is a test-case that fails with the "fix" from #4792 . It fails because depending on the built the ctypes.cdll.mscvrt is outdated and contains other constants than the msvcrt that python uses. These constants are hidden since msvc 14 and are unlikely to be exported again.

I'll submit another commit shortly that (hopefilly) shows that the extra-functions are unnecessary since there is no lost "a"-mode anymore and even if a file-handle would lose it it's unlikely to be recovered by the current function.

MSeifert04 · 2016-04-20T08:41:09Z

I've pushed a "fix" and included a test that the extra function calls are unnecessary. Since this might be objectionable I haven't deleted these functions yet but included a test with Warning in case I've missed some input that actually looses his "a" (append) mode.

Since I had problems setting up the tests with file-creation and teardown in the io.fits.tests.test_util.py file I've not included them yet. But I've tested the following cases:

from astropy.io import fits
# Normal fileio-instance opened in append-mode
with open('test.sth', 'ab') as fileobj:
    assert fits.util._is_append_mode_platform(fileobj)

# Strings and urls are passed through
print(fits.util.fileobj_mode('tmp1.fits.gz'))
# None
print(fits.util.fileobj_mode('http://ned.ipac.caltech.edu/dss1B2/By/RMH82_29:I:IIIaJ:dss1.fits'))
# None

# PIL-Images work:
from PIL import Image
fileobj = Image.open('C:/Users/Admin/Pictures/Saved Pictures/355432.jpg')
print(fits.util.fileobj_mode(fileobj))
# rb

# GZIP
import gzip

for num, mode in [(0, 'a'), (0, 'ab'),
                  (0, 'w'), (0, 'wb'),
                  (1, 'x'), (2, 'xb'),
                  (1, 'r'), (2, 'rb')]:
    with gzip.GzipFile('tmp{0}.fits.gz'.format(num), mode) as fileobj:
        print(mode, fits.util.fileobj_mode(fileobj))

import os
os.remove('tmp0.fits.gz')
os.remove('tmp1.fits.gz')
os.remove('tmp2.fits.gz')
# a ab
# ab ab
# w wb
# wb wb
# x xb
# xb xb
# r rb
# rb rb

# Normal file-handles:
for num, mode in [(0, 'ab'),
                  (0, 'wb'),
                  (1, 'xb'),
                  (1, 'rb')]:
    with open('test{0}.dat'.format(num), mode, buffering=0) as f:
        print(mode, fits.util.fileobj_mode(f))

for num, mode in [(0, 'a'), (0, 'ab'),
                  (0, 'w'), (0, 'wb'),
                  (1, 'x'), (2, 'xb'),
                  (1, 'r'), (2, 'rb')]:
    with open('test1{0}.dat'.format(num), mode) as f:
        print(mode, fits.util.fileobj_mode(f))

os.remove('test0.dat')
os.remove('test1.dat')
os.remove('test10.dat')
os.remove('test11.dat')
# ab ab
# wb wb
# xb xb
# rb rb
# a a
# ab ab
# w w
# wb wb
# x x
# xb xb
# r r
# rb rb

And I'm very sorry that the previous PR didn't work out. Maybe it would have worked out but since the calculation was flawed and I always got the result that it's not in append-mode it was certainly not a good PR.

MSeifert04 · 2016-04-20T08:42:38Z

That's amazing ... the first commit did fail because of an unrelated error. 😄 so much for the "proof"...

pllim · 2016-04-20T13:15:55Z


+    def test_append_mode_is_determined_correctly(self, tmpdir):
+        # TODO: Move this test in test_util as soon as I figure out how to
+        # setup a class with tmpdir...


I use temporary directory in astropy/vo/validator/tests/test_validate.py. Something like

import shutil import tempfile class TestConeSearchValidation(object): def setup_class(self): self.out_dir = tempfile.mkdtemp() def teardown_class(self): shutil.rmtree(self.out_dir)

Is this what you are looking for?

That looks promising. I'll try to setup an own test-class for these test. Thank you.

MSeifert04 · 2016-05-01T21:58:58Z

I know this will be a very tough review - it took me quite a while to figure out what happens in there - but I would be very happy if anyone would volunteer. I've tried to make the first two comments of this PR as informative as possible but if something seems strange please ask and I'll try to explain.

astrofrog · 2016-05-02T07:06:56Z

+    # TODO: This is probably superfluous the old corner cases where FILEIO
+    # objects didn't keep track of their append-flag are over (?)
+    # At least the bugfix was ported to python 2.7 and 3.3+:
+    # http://bugs.python.org/issue18876


Just for info, we do indeed only support Python 2.7 and 3.3+

the warning is for cases I might have forgotten (the most common ones: pil, gzip, fileio, hdulist are covered in the tests) that still forget if they are opened in append-mode. These shouldn't exist anymore but better to have some kind of grace-period where users might get this warning.

pllim · 2016-05-02T15:04:27Z

@embray ?

mhvk · 2016-05-03T14:08:08Z

I looked over the code and it seems OK to me, but I have to admit I don't understand the guts too well. I do think in general it is a good idea to try to simplify rather than leave code in place as we feel we do not understand it well enough to be sure we can remove it. If we are wrong, this is of course unfortunate, but then at least we can have a test case that ensures it keeps being right (and I think the new test cases here are quite exhaustive!).

MSeifert04 · 2016-05-04T18:50:21Z

@mhvk Thank you for reviewing it. So you would suggest removing the (probably) unnessary functions?

mhvk · 2016-05-04T19:05:08Z

So you would suggest removing the (probably) unnecessary functions?

Yes.

MSeifert04 · 2016-05-04T19:32:08Z

So you would suggest removing the (probably) unnecessary functions?

Yes

I'm not sure: Should I just go ahead and delete it or wait?

mhvk · 2016-05-04T19:41:26Z

Ideally, @astrofrog pipes in too, but I think you added such good testing that I'm comfortable merging it (with -- I hope I won't come to regret it -- the unnecessary functions removed -- I think we should understand for all our code why it is there).

MSeifert04 · 2016-05-05T23:03:48Z

Ok if no one disputs your opinion I'll remove the (probably) unnecessary function that manually checks if a file is opened in append mode and do a push sometime this weekend.

I hope you don't mind that I ping you: @embray @astrofrog @eteq

MSeifert04 · 2016-05-07T17:17:26Z

I think somewhere during this PR I've messed something up or did some dubious merge. I had to force push this commit. Probably because I did some local merges with test-branches.

How do I resolve such problems? Is there a general way?

mhvk · 2016-05-07T19:47:20Z

@MSeifert04 - I don't think there is anything wrong with force push. At least, I do them all the time; I think one more or less has to if one rebases on master, or amends a commit.

MSeifert04 · 2016-05-08T12:56:02Z

@mhvk Okay, I was amending so that explains it. Thank you

astrofrog · 2016-05-10T09:25:21Z

+    test_core.TestConvenienceFunctions.test_fileobj_mode_guessing
+    but added some low-level tests as well.
+
+    TODO: These are regression tests for


Why is this marked as a TODO?

Because first I had no regression test for the workaround fail and made this TODO, then I inserted the regression test but after @mhvk said I should delete the "unnecessary" functions I deleted the test again. Long story short: I'll delete the TODO. I thought I caught all of these in the recent cleanup commit. Thanks

astrofrog · 2016-05-10T09:27:34Z

Apart from my comment above, I think this looks good.

@embray - would you be able to just confirm that this is ok?

MSeifert04 · 2016-05-12T23:54:18Z

@astrofrog do I need to write a Changelog? or for that matter will it be part of 1.2?

MSeifert04 · 2016-05-20T14:46:04Z

@embray Do you think removing this workaround is ok or could there still be objects which not track their append-flag? I tested the most common ones of FileIO but also gzip, PIL and HDULists. I'm not sure if I should test with StringIO and BytesIO ... could they behave different?

@astrofrog I don't want to disturb you but will this be in 1.2 and does it need a changelog?

astrofrog · 2016-06-11T08:57:29Z

Sorry that I let this slip through - I think this does need a quick look by @embray or someone else familiar with io.fits before we can merge.

@embray - does this look ok?

MSeifert04 · 2016-07-28T15:13:13Z

This has been "ready-for-final-review" for a while now. Anyone interested in doing that final review?

MSeifert04 · 2016-09-01T18:22:08Z

bump

pllim · 2016-09-01T18:24:02Z

I think we're still waiting for a final nod from @embray

MSeifert04 · 2016-09-21T12:51:30Z

Should I rebase this now or wait for the nod/review?

pllim · 2016-09-21T13:20:18Z

Maybe rebase first, in case you need to change something due to conflict?

- included an roundtripping test that the extra functions might be unneccessary.

MSeifert04 · 2016-09-21T14:08:33Z

was just a trivial change that triggered the conflict.

MSeifert04 · 2016-09-21T16:54:15Z

@pllim Could you restart the coverage test? It got stuck inside some vo test which is probably unrelated.

On a side note: I could have sworn I was able to restart jobs on Travis last week. 😕

pllim · 2016-09-21T16:56:16Z

@MSeifert04 , it is restarted. Are you logged into Travis? Sometimes you have to log in again. Otherwise, maybe @astrofrog can check if you still have access.

astrofrog

This has been open for a while, and no one has objected. I agree with @mhvk that the testing is extensive enough that I think it's unlikely this will break anything on supported versions of Python. Even in the unlikely case that this produces regressions, we do still have the option of reverting the changes later on, so I think we should go ahead and merge.

astrobot · 2016-09-29T10:54:21Z

@astrofrog - thanks for merging this! However, I noticed the following issues with this pull request:

The milestone has not been set
Changelog entry not present (or pull request number missing) and neither the Affects-dev nor the no-changelog-entry-needed label are set

Would it be possible to fix these? Thanks!

This is an experimental bot being written by @astrofrog - let me know if the message above is incorrect!

astrofrog · 2016-09-29T10:54:29Z

Thanks @MSeifert04!

astrofrog · 2016-09-29T10:59:29Z

Changelog entry added in f8aa9f8

MSeifert04 · 2016-09-29T19:46:04Z

👍 🎉

@astrofrog Thanks for adding the changelog entry.

MSeifert04 changed the title ~~proof that the current workaround does not work correctly~~ proof that the current workaround for appveyor python 3.5 does not work correctly Apr 20, 2016

MSeifert04 changed the title ~~proof that the current workaround for appveyor python 3.5 does not work correctly~~ current workaround #4792 for appveyor python 3.5 does not work correctly Apr 20, 2016

pllim added io.fits Affects-release labels Apr 20, 2016

pllim reviewed Apr 20, 2016
View reviewed changes

astrofrog reviewed May 2, 2016
View reviewed changes

astrofrog reviewed May 10, 2016
View reviewed changes

astrofrog added the Ready-for-final-review label May 10, 2016

MSeifert04 added 9 commits September 21, 2016 15:59

proof that the current workaround does not work correctly

8b67540

avoiding the issue by special casing python 3.5

d483f8d

- included an roundtripping test that the extra functions might be unneccessary.

added tests and set Warning-Class

9eb0aa8

fixed python version problems in tests

d7e856c

fixed tests (again)

5f4a9f2

inline documentation update

8ba97dd

removed manual append mode determination

2c541c8

removed _fileobj_is_append_mode function too

c546ab6

inline documentation cleanup

841a80f

astrofrog approved these changes Sep 29, 2016

View reviewed changes

astrofrog merged commit c8080dd into astropy:master Sep 29, 2016

astrofrog added this to the v1.3.0 milestone Sep 29, 2016

MSeifert04 deleted the io_fits_append_mode branch September 29, 2016 10:56

astrofrog added a commit that referenced this pull request Sep 29, 2016

Added changelog entry for #4793 [ci skip]

f8aa9f8

MSeifert04 mentioned this pull request Nov 28, 2016

Pyfits getdata creating a possible Python version 3 error #5510

Closed

Conversation

MSeifert04 commented Apr 20, 2016

Uh oh!

MSeifert04 commented Apr 20, 2016

Uh oh!

MSeifert04 commented Apr 20, 2016

Uh oh!

pllim Apr 20, 2016

Choose a reason for hiding this comment

Uh oh!

MSeifert04 Apr 20, 2016

Choose a reason for hiding this comment

Uh oh!

MSeifert04 commented May 1, 2016

Uh oh!

astrofrog May 2, 2016

Choose a reason for hiding this comment

Uh oh!

MSeifert04 May 2, 2016

Choose a reason for hiding this comment

Uh oh!

pllim commented May 2, 2016

Uh oh!

mhvk commented May 3, 2016

Uh oh!

MSeifert04 commented May 4, 2016

Uh oh!

mhvk commented May 4, 2016

Uh oh!

MSeifert04 commented May 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented May 4, 2016

Uh oh!

MSeifert04 commented May 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MSeifert04 commented May 7, 2016

Uh oh!

mhvk commented May 7, 2016

Uh oh!

MSeifert04 commented May 8, 2016

Uh oh!

astrofrog May 10, 2016

Choose a reason for hiding this comment

Uh oh!

MSeifert04 May 10, 2016

Choose a reason for hiding this comment

Uh oh!

astrofrog commented May 10, 2016

Uh oh!

MSeifert04 commented May 12, 2016

Uh oh!

MSeifert04 commented May 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astrofrog commented Jun 11, 2016

Uh oh!

MSeifert04 commented Jul 28, 2016

Uh oh!

MSeifert04 commented Sep 1, 2016

Uh oh!

pllim commented Sep 1, 2016

Uh oh!

MSeifert04 commented Sep 21, 2016

Uh oh!

pllim commented Sep 21, 2016

Uh oh!

MSeifert04 commented Sep 21, 2016

Uh oh!

MSeifert04 commented Sep 21, 2016

Uh oh!

pllim commented Sep 21, 2016

Uh oh!

astrofrog left a comment

Choose a reason for hiding this comment

Uh oh!

astrobot commented Sep 29, 2016

Uh oh!

astrofrog commented Sep 29, 2016

MSeifert04 commented May 4, 2016 •

edited

Loading

MSeifert04 commented May 5, 2016 •

edited

Loading

MSeifert04 commented May 20, 2016 •

edited

Loading