BUG: warn instead of raise when failing to read specific files in fitsdiff script by neutrinoceros · Pull Request #18882 · astropy/astropy

neutrinoceros · 2025-11-11T16:22:14Z

Description

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2025-11-11T16:22:26Z

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

github-actions · 2025-11-11T16:22:27Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

pllim · 2025-11-11T16:59:56Z

devdeps, devinfra, and predeps failures appear unrelated

pllim · 2025-11-11T18:31:23Z

Which test was failing where before? I was expecting a test to be modified to catch this added warning but I don't see it in the code and CI is green. Hope you can clarify. Thanks!

neutrinoceros · 2025-11-11T18:39:39Z

the test runs as is. The reprod listed on the issue #18880

What's surprising is that it seems I discovered the issue with #18784
My hypothesis is that having pytest-astropy as a dependency indirectly made us pull uncompresspy in CI, which hid the bug for 6 months, and that removing it revealed the issue.
If my guess is correct, then the test is either skipped or already passing in CI. I suggest manual testing to truly validate the fix. Just make sure that uncompresspy isn't installed and run the test directly through pytest.

neutrinoceros · 2025-11-12T08:06:47Z

I think my previous hypothesis was incorrect. Rather, I now think that the important (yet, unintended) change in #18784 is the behavior of the slow marker; the test isn't skipped any more (even though it should), which would explain why it showed on my radar but we never caught it before.

saimn · 2025-11-12T08:41:30Z

--run-slow is used only in 2 jobs which also have optional deps... so no job was supposed to fail.
This test is not that slow (<1s on my laptop, so more on CI but probably still reasonable), so maybe we can remove the marker. ~~You would need to catch the warning is uncompresspy is not installed.~~ Not a real warning, ok. Maybe good to check stderr then.

neutrinoceros · 2025-11-12T08:51:31Z

@saimn I tentatively removed the marker.
I tried to respect the -q flag, and the test uses it, so there's currently nothing to capture in stderr. Is there anything you think should change (be it the test or the implementation) ?

saimn · 2025-11-12T10:45:03Z

Right. Would still be useful to test that the error is properly caught and message is correctly formatted and printed. Maybe easier to do with a dedicated test using only the .Z file in some tmp directory ?

neutrinoceros · 2025-11-12T10:57:29Z

sure, coming right up.

neutrinoceros · 2025-11-12T11:06:45Z

test added

neutrinoceros · 2025-11-12T11:11:22Z

actually I'll take another minute to polish it. Will undraft when ready

saimn · 2025-11-12T11:17:36Z

astropy/io/fits/tests/test_fitsdiff.py

+        out, err = capsys.readouterr()
+        assert out == ""
+        assert err == f"Warning: failed to open {tmp} (or {Zfile}). Skipping.\n"
+


Thanks. To be complete, could you also test ~~when b != a~~ and when -q is used?
Also we could make the test independent of whether uncompresspy is installed or not by moking its import (I think this can be done applying pytest's monkeypatch on sys.modules).

Thanks. To be complete, could you also test when b != a and when -q is used?

done. will push in a minute.

Also we could make the test independent of whether uncompresspy is installed or not by moking its import (I think this can be done applying pytest's monkeypatch on sys.modules).

I'm not sure about this; my understanding is that sys.modules reflects what modules where already imported, while HAS_UNCOMPRESSPY is only about wether the module is installed (wether or not it was already imported). Maybe I can mock the constant itself, but I've had bad experiences with mocking so I'm not confident this will work.

what I tried:

monkeypatch.setattr( "astropy.utils.compat.optional_deps", "HAS_UNCOMPRESSPY", False )

result:

AttributeError: 'astropy.utils.compat.optional_deps' has no attribute 'HAS_UNCOMPRESSPY'

I never quite seem to get the hang of it. I'm open to suggestions. Meanwhile, I'll push what working code I have.

Indeed, but this seems to work (and is more targeted so probably better anyway):

monkeypatch.setattr(fits.file, "HAS_UNCOMPRESSPY", False)

confirmed. Thank you very much !

neutrinoceros · 2025-11-12T11:31:20Z

Ah. My tests are failing on a warning despite it being explicitly filtered. I can only suspect this has to do with -p no:warnings, which is addressed in #18890...

neutrinoceros · 2025-11-12T14:29:56Z

tentatively rebased on top of #18890

neutrinoceros · 2025-11-12T15:47:49Z

that seems to do it. I'll mark that other PR as a blocker to this one then

…sdiff script

neutrinoceros · 2025-11-13T13:31:29Z

rebased to main. Should be stable now.

saimn

LGTM, thanks.

… read specific files in fitsdiff script

…882-on-v7.2.x Backport PR #18882 on branch v7.2.x (BUG: warn instead of raise when failing to read specific files in fitsdiff script)

neutrinoceros added this to the v7.2.0 milestone Nov 11, 2025

github-actions bot added the io.fits label Nov 11, 2025

neutrinoceros added the backport-v7.2.x on-merge: backport to v7.2.x label Nov 11, 2025

neutrinoceros force-pushed the io/fits/bug/diffscript-skip-on-error branch from fca1854 to def9770 Compare November 11, 2025 16:23

neutrinoceros added the Bug label Nov 11, 2025

neutrinoceros marked this pull request as ready for review November 11, 2025 16:34

neutrinoceros requested a review from saimn as a code owner November 11, 2025 16:34

pllim added Extra CI Run cron CI as part of PR Build all wheels Run all the wheel builds rather than just a selection labels Nov 11, 2025

neutrinoceros mentioned this pull request Nov 12, 2025

TST/DEP: drop pytest-astropy in favor of corresponding, direct test dependencies #18784

Draft

1 task

neutrinoceros force-pushed the io/fits/bug/diffscript-skip-on-error branch from 55d1452 to ad16d47 Compare November 12, 2025 11:06

neutrinoceros marked this pull request as draft November 12, 2025 11:11

neutrinoceros force-pushed the io/fits/bug/diffscript-skip-on-error branch from ad16d47 to a806ef5 Compare November 12, 2025 11:16

neutrinoceros marked this pull request as ready for review November 12, 2025 11:16

saimn reviewed Nov 12, 2025

View reviewed changes

neutrinoceros force-pushed the io/fits/bug/diffscript-skip-on-error branch from a806ef5 to 2553087 Compare November 12, 2025 11:28

neutrinoceros force-pushed the io/fits/bug/diffscript-skip-on-error branch from 2553087 to 44e83c3 Compare November 12, 2025 12:44

neutrinoceros force-pushed the io/fits/bug/diffscript-skip-on-error branch 2 times, most recently from d370e96 to 91fc20b Compare November 12, 2025 14:29

neutrinoceros marked this pull request as draft November 12, 2025 14:30

neutrinoceros changed the title ~~BUG: warn instead of raise when failing to read specific files in fitsdiff script~~ BUG: warn instead of raise when failing to read specific files in fitsdiff script (blocked by #18890) Nov 12, 2025

BUG: warn instead of raise when failing to read specific files in fit…

332b65b

…sdiff script

neutrinoceros force-pushed the io/fits/bug/diffscript-skip-on-error branch from 91fc20b to 332b65b Compare November 13, 2025 13:30

neutrinoceros changed the title ~~BUG: warn instead of raise when failing to read specific files in fitsdiff script (blocked by #18890)~~ BUG: warn instead of raise when failing to read specific files in fitsdiff script Nov 13, 2025

neutrinoceros marked this pull request as ready for review November 13, 2025 13:31

neutrinoceros requested a review from saimn November 13, 2025 13:31

saimn approved these changes Nov 13, 2025

View reviewed changes

saimn merged commit 4e5b792 into astropy:main Nov 13, 2025
50 of 51 checks passed

meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request Nov 13, 2025

Backport PR astropy#18882: BUG: warn instead of raise when failing to…

198ed29

… read specific files in fitsdiff script

meeseeksmachine mentioned this pull request Nov 13, 2025

Backport PR #18882 on branch v7.2.x (BUG: warn instead of raise when failing to read specific files in fitsdiff script) #18907

Merged

neutrinoceros deleted the io/fits/bug/diffscript-skip-on-error branch November 13, 2025 18:11

neutrinoceros added a commit to meeseeksmachine/astropy that referenced this pull request Nov 14, 2025

Backport PR astropy#18882: BUG: warn instead of raise when failing to…

128db9c

… read specific files in fitsdiff script

astrofrog pushed a commit to meeseeksmachine/astropy that referenced this pull request Nov 14, 2025

Backport PR astropy#18882: BUG: warn instead of raise when failing to…

f85f17f

… read specific files in fitsdiff script

astrofrog added a commit that referenced this pull request Nov 14, 2025

Merge pull request #18907 from meeseeksmachine/auto-backport-of-pr-18…

4136ed1

…882-on-v7.2.x Backport PR #18882 on branch v7.2.x (BUG: warn instead of raise when failing to read specific files in fitsdiff script)

Uh oh!

Conversation

neutrinoceros commented Nov 11, 2025

Description

Uh oh!

github-actions bot commented Nov 11, 2025

Uh oh!

github-actions bot commented Nov 11, 2025

Uh oh!

pllim commented Nov 11, 2025

Uh oh!

pllim commented Nov 11, 2025

Uh oh!

neutrinoceros commented Nov 11, 2025

Uh oh!

neutrinoceros commented Nov 12, 2025

Uh oh!

saimn commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neutrinoceros commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saimn commented Nov 12, 2025

Uh oh!

neutrinoceros commented Nov 12, 2025

Uh oh!

neutrinoceros commented Nov 12, 2025

Uh oh!

neutrinoceros commented Nov 12, 2025

Uh oh!

saimn Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

saimn Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neutrinoceros commented Nov 12, 2025

Uh oh!

neutrinoceros commented Nov 12, 2025

Uh oh!

neutrinoceros commented Nov 13, 2025

Uh oh!

saimn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

saimn commented Nov 12, 2025 •

edited

Loading

neutrinoceros commented Nov 12, 2025 •

edited

Loading

neutrinoceros commented Nov 12, 2025 •

edited

Loading