Fix fitscheck and checksums correction by saimn · Pull Request #6571 · astropy/astropy

saimn · 2017-09-19T22:12:28Z

Fix #6530, #5902, #5874, and replaces #5875

Remove the nonstandard option (ref fitscheck checksum corrections don't work #6530 (comment))
Fix the fitscheck script that was broken in multiple ways: it is based on the detection of warnings emitted when the checksum is invalid, but putting these warings in error mode also means that the files could not be opened without errors to be fixed. Then it was not possible to write new checksum replacing invalid one, so now I store the result of the verification to know if the checksum must be overwritten.
Also remove a few unused things, and add tests, yay!

It's mostly bugfixes, so should go to v2.0, but I also removed the nonstandard option, so not sure... thoughts @pllim @MSeifert04 @bsipocz ?

astropy-bot · 2017-09-19T22:12:46Z

Hi there @saimn 👋 - 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 labelled 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! 👍

bsipocz · 2017-09-19T22:42:32Z

Could you refactor the removal of nonstandard into another PR? If yes, then this can go into 2.0.x, otherwise I would say 3.0

MSeifert04 · 2017-09-19T22:43:08Z

The blocking removal actually makes it a breaking-change. I don't know if we need a deprecation-period but I would say it definitely has to wait for a new not-bugfix-release.

Even though some test failures are unrelated some seem real, any idea why?

saimn · 2017-09-20T08:19:34Z

For test failures, I think there is a bad interaction between logging and the capsys plugin. I'm trying another option to capture the output.
About nonstandard, I should have thought about this earlier 😢 . I will try to isolate this commit, otherwise it will be for 3.0.

saimn · 2017-09-20T09:24:41Z

Hmm, of course the subprocess way cannot work since entrypoints are not installed in the temporary directory...

MSeifert04 · 2017-09-20T10:40:11Z

+
+
+class TestFitscheck(FitsTestCase):
+    # def setup(self):


Just as a reminder to remove this commented code again ... later.

Oops yeah, once I find a proper way to capture stderr I will cleanup 😉

Can't you just assign to sys.stderr?

That's what capsys do, but it was failing : there are issues specific to windows and capturing stderr (and it still fails with the last try), and probably a real failure on python 2 (on CircleCI).

Ah, do you know which commit (test log would be interesting) that was?

The commit using capsys is 316fef2 , and appveyor is linked below.

MSeifert04 · 2017-09-20T10:53:52Z

Most of the changes look fine, but I've skipped the docs for now.

I'm really not sure what's a good way to check for emitted logging messages. But I think there's probably an easier solution that to redirect stderr with a context manager.

Maybe @astrofrog or @bsipocz know a more elegant way?

saimn · 2017-09-20T10:59:40Z

@MSeifert04 - In case you are interested in the windows failures, this is the build when using capsys:
https://ci.appveyor.com/project/Astropy/astropy/build/1.0.8578#L3046

Traceback (most recent call last):
  File "C:\conda\envs\test\lib\logging\__init__.py", line 994, in emit
    stream.write(msg)
ValueError: I/O operation on closed file.
Call stack:
...

It seems that redirecting stderr prevents the logger to work correctly.

MSeifert04 · 2017-09-20T11:13:26Z

That's interesting because it seems to work with fitsdiff. Is it possible that the setup_logger in fitscheck does someting weird or misses something essential that the setup_logger in fitsdiff does?

saimn · 2017-09-20T12:56:52Z

One difference with fitsdiffis that it uses logging only for warnings/errors, but the diff reports are printing directly to stdout. So it's possible that existing tests does not trigger these logging messages.

pllim · 2017-09-20T13:33:11Z

FWIW it does include API change, so maybe should just be in v3 (one more reason for people to upgrade).

c/c @jaytmiller

saimn · 2017-09-20T15:20:54Z

Oh, Windows ....

FileExistsError: [WinError 183] Cannot create a file when that file already exists: 'C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\fits-test-8n8hve89\\checksum.fits.bak' -> 'C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\fits-test-8n8hve89\\checksum.fits'

Aside from the logging issue, there is also one with the update which was overwriting the file when it was still open. Ok fair enough, I now write the file to a temp one before renaming it, and this is the result. @MSeifert04 - any idea on how to best solve this ?

MSeifert04 · 2017-09-21T00:25:21Z

I really don't know what's the problem there. It seems like a weird interaction between capsys and astropy.tests.catch_warnings (it works better with warnings.catch_warnings). However that's totally convoluted and I really don't know where to start looking for a better solution...

The interesting question is: Is that windows specific or a real issue?

- Remove unused _checksum_comment and _datasum_comment attributes - Add _datasum_valid and _checksum_valid to store the validity flag computed when the file is opened. And use this to know if the checksums are valid or must be recomputed when writing.

saimn · 2017-09-22T09:07:59Z

Rebased and tests pass (I skipped the stderr based checks on win32), so please review @MSeifert04 (and @drdavella ?).

The interesting question is: Is that windows specific or a real issue?

Not sure, there is clearly something specific to windows. When running tests with the pytest capture mode disable (-s) and use pdb I can see similar errors, but it's not surprising that this can conflict with capsys. On windows it seems that this conflict is always there.
It's always a pain to test logging messages, maybe pytest-catchlog could help here.

pllim · 2017-09-22T13:51:59Z

Re: Windows. Maybe I should test this on my Windows laptop before merge?

drdavella · 2017-09-22T13:05:44Z

+                        return 1
+
+    for w in wlist:
+        if str(w.message).startswith(('Checksum verification failed',


This would maybe be a bit cleaner if we used a new warning subclass for all FITS checksum problems instead of just AstropyUserWarning. In that case the new warning class could be passed to catch_warnings above.

drdavella · 2017-09-22T13:07:10Z

+        if str(w.message).startswith(('Checksum verification failed',
+                                      'Datasum verification failed')):
+            log.warning('BAD %r %s', filename, str(w.message))
+            return 1


I realize this is the same behavior as the old code, but wouldn't it be more useful to not fail immediately after the first problem is encountered so that all checksum problems are reported? The same goes for the loop above that looks for missing checksums.

drdavella · 2017-09-22T13:08:59Z

    """

-    hdulist = fits.open(filename, do_not_scale_image_data=True)
+    tmpfile = filename + '.bak'


Would python's tempfile module be useful here?

No strong preference for me (I did that quickly to debug the windows issue). One advantage of not using /tmp is that it can avoid a copy to another disk, e.g. if your data is on a NFS share.

It's really bugging me that we need a temporary file at all. Is there no way to run a complete check without actually writing it to a file? Or is the intention here to fix the problems?

Sorry I haven't had the chance to look into this more closely, but I'll do so next week.

The goal here is to update the file with new checksums, and windows does not allow to overwrite the file while it is open.

Then why not read, close then write?

Because there is no nice way to force reading the data (you need to iterate on all HDUs and access the .data attribute). The other (better) option would be to use the update mode and .flush, I think it can handle the checksums, but there may be a good reason why it is not used. So, at least for now, the tmp file and replace solution seems the most straightforward.

drdavella · 2017-09-22T13:35:00Z

-            self._datasum_comment = self._header.comments['DATASUM']
-
-            if not self.verify_datasum(blocking):
+            self._datasum_valid = self.verify_datasum()


The datasum gets computed twice since it is also computed within verify_checksum. It would be nice if there was a way to store the datasum value between calls so that it does not have to be recomputed.

drdavella · 2017-09-22T13:45:45Z

-            self._checksum_comment = self._header.comments['CHECKSUM']
-            if not self.verify_checksum(blocking):
+            self._checksum_valid = self.verify_checksum()
+            if not self._checksum_valid:


The code is correct, and it's perfectly valid Python, but in terms of style and clarity it bothers me a little bit that _checksum_valid and _datasum_valid are being used as booleans, even though verify_checksumreturns integer values. It would be a little more explicit to have the line above be something like
self._checksum_valid = self._verify_checksum() == 1

Even better would to have named constants for the return values of verify_checksum and verify_datasum:

CHECKSUM_INVALID = 0 CHECKSUM_VALID = 1 CHECKSUM_MISSING = 2

drdavella · 2017-09-22T13:52:21Z

@@ -526,25 +526,13 @@ def _update_checksum(self, checksum, checksum_keyword='CHECKSUM',
                del self._header[datasum_keyword]
        elif (modified or self._new or
                (checksum and ('CHECKSUM' not in self._header or


It's not really clear to me, but should 'CHECKSUM' and 'DATASUM' instead be checksum_keyword and datasum_keyword?

drdavella · 2017-09-22T13:55:10Z

-        # used by the utility script "fitscheck" to detect missing
-        # checksums.
-
        if 'CHECKSUM' in self._header:


Not a big problem, but it would be nice to avoid doing this check more than once since it also happens in verify_checksum. This could potentially be refactored since verify_checksum will return 2 if the 'CHECKSUM' keyword is missing. Same goes for the check for 'DATASUM' below.

drdavella · 2017-09-22T13:59:14Z

@saimn these changes look good and I don't think there are any issues that would block a merge. I have a lot of comments that are mostly unrelated to your changes but instead relate to the overall state of this code. Like most of the FITS code, it could stand to be improved.

In general, I wonder if every PR that involves FITS code should be used as an opportunity to clean up the code that is being touched, even if it does not directly affect the issue being addressed. I realize this makes reviews more difficult, but if we don't make some attempt to incrementally improve this code, it is going to continue to be very difficult to maintain.

pllim · 2017-09-22T14:03:16Z

I wonder if every PR that involves FITS code should be used as an opportunity to clean up the code

That is a bit too ambitious. I propose that we table that thought until we have an official lead maintainer for io.fits. FITS is always a can of worms. You think you might be improving it but then end up breaking some obscure edge case. Just sayin'.

saimn · 2017-09-22T21:14:37Z

@drdavella - Your remarks are valid, but I tried to stay close to the original code as the goal is to fix the bugs and test that it works. I was also hoping to backport this to v2.0.x (it should be feasible, not sure I will do it though).
Cleaning up the code by the same occasion is too hazardous, it's easy to miss something and break a "feature" that is not correctly tested. But of course if you want to improve the code in a follow-up PR it would be really great (as you said the current way to test if checksums are valid is really not optimal).

pllim · 2017-09-22T21:39:40Z

@saimn, if you want me to test this on my Windows laptop, just let me know. Sounds like this PR still might change, so I'll hold off for now.

saimn · 2017-09-22T21:46:31Z

@pllim - Yes if you want to, it would be useful! (you need to remove the on_win32 check in test_fitscheck.py [EDITED] to enable the stderr assertions). I don't want to refactor the checksum code in this PR, so if we can find a better way to test on Windows it would be great.

MSeifert04 · 2017-09-23T00:38:01Z

I'll also have a look at the windows issues (although next week). I don't think it's a particular problem with the PR. I suspect that logging (or astropy-logging) somehow interferes with capsys...

pllim · 2017-09-23T15:00:27Z

So... I can't reproduce this on my Windows laptop.

>>> import sys
>>> sys.platform
'win32'

I changed test_fitscheck.py from this PR locally to pretend I am not on Windows:

And python setup.py test -P io.fits all passed:
xlog.txt

Let's wait and see what @MSeifert04 find on his side.

drdavella · 2017-09-23T15:29:59Z

@saimn, @pllim, you're both right of course; I was just trying to think of ways that we could possibly reduce the maintenance burden of io.fits incrementally without doing it in one massive overhaul.

I can approve the review if needed but maybe we should wait for @MSeifert04.

MSeifert04 · 2017-09-23T15:54:15Z

@saimn Could you undo the if on_win32 change? Would be interesting to know what exception happened.

saimn · 2017-09-23T16:16:13Z

Thanks for the test @pllim, interesting result! It may then be specific to appveyor.

@MSeifert04 - I have only my phone until tomorrow, but I guess you could find the failing build in appveyor history, or you can update my branch if you prefer. But it may not be useful to spend too much time on this issue: fitscheck is tested on other platforms, and the checksum code is tested as well. My only worry is that testing only the error numbers is fragile, but it should be ok.

MSeifert04 · 2017-09-24T14:21:43Z

As far as I can see there was no build with the replace but without the on_win32. Locally it worked without it (complete test suite) so I added a commit. Let's see what AppVeyor thinks.

MSeifert04

LGTM!

I still think the write to temporary and then replace is kind of ugly but I don't think it should hold up the PR. If you have an idea how it could be solved differently feel free to address this. Otherwise I'm also fine if we leave it as-is.

saimn · 2017-09-25T10:40:39Z

As far as I can see there was no build with the replace but without the on_win32. Locally it worked without it (complete test suite) so I added a commit. Let's see what AppVeyor thinks.

Oh great! You're right I probably didn't test this on Appveyor after fixing the file update.

I still think the write to temporary and then replace is kind of ugly but I don't think it should hold up the PR. If you have an idea how it could be solved differently feel free to address this.

Agreed about the ugliness, as said above I think .flush should work but I'm not completely sure it handles all cases the same way. I pushed a commit to try this solution.

saimn · 2017-09-25T11:06:59Z

It seems to work, good news 😂 ! And the test failure is not related (see #6604), so let's move forward ? ( @MSeifert04 - it seems you have new powers, if you want to press on the green button ? 🙏 )

MSeifert04 · 2017-09-25T11:27:30Z

Thanks @saimn !

I really like the flush option.

saimn added the io.fits label Sep 19, 2017

saimn force-pushed the checksum branch from 186fb35 to f5e4aa2 Compare September 20, 2017 09:51

MSeifert04 reviewed Sep 20, 2017

View reviewed changes

saimn force-pushed the checksum branch from f9c3079 to e0b02aa Compare September 20, 2017 13:49

saimn added this to the v3.0.0 milestone Sep 20, 2017

saimn force-pushed the checksum branch from e25683d to 1050ea4 Compare September 20, 2017 14:49

saimn force-pushed the checksum branch from f82d782 to 126c759 Compare September 20, 2017 19:46

saimn added 11 commits September 22, 2017 09:58

Remove unused checksum cases (test, nonstandard_datasum)

caf8053

Add test for invalid checksums

cd8235b

Fix warnings detection in fitscheck

2680e30

Remove the nonstandard blocking option for checksums

d4fff92

Add tests for fitscheck

40a4604

Fix the remove option

951a4e7

Fix warnings catching with py2

3cf5a22

Avoid overwriting file while it is still open, windows don't like this

8bf5f56

This should make windows happy (py3 only)

d1da3e4

Avoid to use capsys for win32

06db9e3

Add changelog entry

c255248

saimn force-pushed the checksum branch from 126c759 to c255248 Compare September 22, 2017 07:59

saimn requested a review from drdavella September 22, 2017 08:59

drdavella reviewed Sep 22, 2017

View reviewed changes

remove on_win cases in tests

5edfbc9

MSeifert04 approved these changes Sep 24, 2017

View reviewed changes

Use flush to update the file with new checksums

b3bc11a

MSeifert04 merged commit 11938b7 into astropy:master Sep 25, 2017

saimn deleted the checksum branch September 25, 2017 11:30

This was referenced Sep 25, 2017

script 'fitscheck' not deleting CHECKSUM and DATASUM keywords #5874

Closed

Fix for issue #5874 for fitscheck script not deleting CHECKSUM and DA… #5875

Closed

saimn mentioned this pull request Sep 25, 2017

Bad checksum not being overwritten on call to "writeto" #5902

Closed

Conversation

saimn commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astropy-bot Bot commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsipocz commented Sep 19, 2017

Uh oh!

MSeifert04 commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saimn commented Sep 20, 2017

Uh oh!

saimn commented Sep 20, 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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 commented Sep 20, 2017

Uh oh!

saimn commented Sep 20, 2017

Uh oh!

MSeifert04 commented Sep 20, 2017

Uh oh!

saimn commented Sep 20, 2017

Uh oh!

pllim commented Sep 20, 2017

Uh oh!

saimn commented Sep 20, 2017

Uh oh!

MSeifert04 commented Sep 21, 2017

Uh oh!

saimn commented Sep 22, 2017

Uh oh!

pllim commented Sep 22, 2017

Uh oh!

drdavella Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drdavella commented Sep 22, 2017

Uh oh!

pllim commented Sep 22, 2017

Uh oh!

saimn commented Sep 22, 2017

Uh oh!

saimn commented Sep 19, 2017 •

edited

Loading

astropy-bot Bot commented Sep 19, 2017 •

edited

Loading

MSeifert04 commented Sep 19, 2017 •

edited

Loading

MSeifert04 Sep 20, 2017 •

edited

Loading

drdavella Sep 22, 2017 •

edited

Loading

saimn commented Sep 22, 2017 •

edited by pllim

Loading

drdavella commented Sep 23, 2017 •

edited

Loading

MSeifert04 commented Sep 23, 2017 •

edited

Loading