BUG: skip un-portable hack on windows + numpy2 by neutrinoceros · Pull Request #16581 · astropy/astropy

neutrinoceros · 2024-06-17T09:16:41Z

Description

This is meant to fix #16578

The problematic block of code is clearly marked as a hack and the long comment goes on to explain that it shouldn't even be needed in most scenarios, so I think it might be okay to just skip the hack entirely on numpy 2.0 where it's not applicable. Let's see what CI has to say.

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 · 2024-06-17T09:17:06Z

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 · 2024-06-17T09:17:09Z

👋 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?

neutrinoceros · 2024-06-17T09:41:11Z

CI actually pass, which means none of our tests actually relies on the hack, so hopefully our users do not either 🤞🏻

astrofrog

This might indeed be the simplest approach, and we can revisit if users on Windows report issues again related to this?

neutrinoceros · 2024-06-17T10:59:07Z

Definitely. I should point out that this hack has been with us for 12 years so it's actually possible that some end users or even downstream packages have been unknowingly relying on it.

mhvk

Yes, I remember the addition of the deprecation warning and not at all being sure even then it was actually needed. For now, this seems the most expedient route - hopefully, we either hear soon it is actually important or find out that it is not required at all.

mhvk · 2024-06-17T11:24:35Z

Let's add a changelog entry so there is a(nother) place to find that something changed if someone runs into a segfault...

neutrinoceros · 2024-06-17T12:06:07Z

changelog added !

neutrinoceros · 2024-06-17T12:55:16Z

@astrofrog Hopefully this can be backported now :)

eerovaher · 2024-06-17T14:17:49Z

@@ -0,0 +1 @@
+Skip an un-portable hack for Windows if NumPy 2.0 or newer is installed.


This is in the bugfix section, so as a user I would be very interested in under what circumstances I might encounter this hack. If we cannot figure out a situation where the hack is relevant then the change is not user-facing and should not be in the change log at all.

I think the idea is that most readers wouldn't care but it's good to have an entry there to signal that we're aware of this change if anyone comes looking for it. Does it make sense ?

The entry is so vague that it is impossible for users to know if they should care or not (if they are on Windows).

Fair enough. How about:

Skip an un-portable hack in for Windows if NumPy 2.0 or newer is installed.
This fixes a crash that occurred for files oopened via fits.open(..., mode='update')

Mentioning fits.open() and mode="update" looks like it might be specific enough, but if the hack can be accessed through the high-level unified I/O interface then that should be mentioned too.

I don't know that it can. fits.open(..., mode="update") just seemed to be the common denominator for all 4 failing tests we had (see example logs)

neutrinoceros · 2024-06-18T07:24:38Z

I updated the changelog so it's hopefully more useful to users. This should go in the newly created v6.1.2 milestone

mhvk · 2024-06-18T08:45:16Z

The readthedocs failure cannot be related - presumably fixed by #16583

mhvk · 2024-06-18T11:34:55Z

@pllim - would you be able to override? The readthedocs failure is unrelated (#16583)

neutrinoceros · 2024-06-18T15:37:43Z

Alternatively, I just rebased now.

eerovaher · 2024-06-18T15:44:33Z

+This fixes a crash that occurred for files oopened via
+``fits.open(..., mode='update')``.


Change log entries should describe to users what has changed in enough detail that looking up the actual code changes or any discussions on GitHub should not be required. If I try to read the two sentences here without any other context I get the question if skipping a hack fixes a crash then does that mean that the hack caused the crashes? If so then why was the hack there in the first place?

does that mean that the hack caused the crashes ?

This would be the correct conclusion.

If so then why was the hack there in the first place?

It's already hard for us to know that. What would you suggest we say instead ?

I'm not familiar with io.fits and I'm not familiar with Windows either, so I don't really know what to say. I'm just pointing out that the change log is meant to be for the benefit of the users so the entries should not confuse them.

How about this ?

Fixed a crash that occurred for files opened via ``fits.open(..., mode='update')``, on Windows, and with numpy 2.0 installed. It is possible, though unlikely, that this patch reveals memory issues for downstream code run under these exact conditions. Please report any regression found.

Darn, sorry, I should have checked more carefully. That substitution is definitely better. Sigh.

mhvk · 2024-06-18T17:38:29Z

OK, everything passed now, so let's get this in too. Thanks, @neutrinoceros!

mhvk · 2024-06-18T18:15:13Z

@@ -0,0 +1,5 @@
+Fixed a crash that occurred for files opened via
+``fits.open(..., mode='update')``, on Windows, and with numpy 2.0 installed.
+It is possible, though unlikely, that this patch reveals memory issues


Ah, actually, what I merged was your updated version. I'm really not quite with it tonight, but at least that means I happened to merge something I think addresses @eerovaher's critique. Though again apologies, @eerovaher, not to let you react first.

The pull request has already been merged, but this looks good enough to me.

…/skip_windows_hack_on_numpy2 BUG: skip un-portable hack on windows + numpy2

saimn · 2024-06-25T10:35:18Z

                # flushing (on Windows--again, this is no problem on Linux).
+                # Note that this hack is only possible on numpy 1.x:
+                # in 2.x, we cannot write directly to the data attribute
                for idx, mmap, arr in mmaps:


We should add a warning if there are references to the mmap array, accessing it will segault ! (which was the reason for this hack).

Good point. I'll give this a try now.

here it is #16608
please let me know if I've accurately implemented what you had in mind

github-actions Bot added io.fits installation labels Jun 17, 2024

neutrinoceros marked this pull request as ready for review June 17, 2024 09:41

neutrinoceros requested a review from saimn as a code owner June 17, 2024 09:41

astrofrog approved these changes Jun 17, 2024

View reviewed changes

mhvk approved these changes Jun 17, 2024

View reviewed changes

neutrinoceros force-pushed the io.fits/bug/16578/skip_windows_hack_on_numpy2 branch from 4e0ba48 to ff49ce9 Compare June 17, 2024 12:06

eerovaher reviewed Jun 17, 2024

View reviewed changes

neutrinoceros force-pushed the io.fits/bug/16578/skip_windows_hack_on_numpy2 branch from ff49ce9 to 640ad04 Compare June 18, 2024 07:23

mhvk added this to the v6.1.2 milestone Jun 18, 2024

mhvk added the Bug label Jun 18, 2024

BUG: skip un-portable hack on windows + numpy2

2be96df

neutrinoceros force-pushed the io.fits/bug/16578/skip_windows_hack_on_numpy2 branch from 640ad04 to 2be96df Compare June 18, 2024 15:37

eerovaher reviewed Jun 18, 2024

View reviewed changes

a (possibly) clearer changelog

239c9d4

neutrinoceros force-pushed the io.fits/bug/16578/skip_windows_hack_on_numpy2 branch from facbc08 to 239c9d4 Compare June 18, 2024 16:25

mhvk merged commit c86243e into astropy:main Jun 18, 2024

neutrinoceros deleted the io.fits/bug/16578/skip_windows_hack_on_numpy2 branch June 18, 2024 17:57

mhvk reviewed Jun 18, 2024

View reviewed changes

neutrinoceros pushed a commit to neutrinoceros/astropy that referenced this pull request Jun 18, 2024

Merge pull request astropy#16581 from neutrinoceros/io.fits/bug/16578…

d20d614

…/skip_windows_hack_on_numpy2 BUG: skip un-portable hack on windows + numpy2

neutrinoceros mentioned this pull request Jun 18, 2024

Backport PR 16581 on branch v6.1.x (BUG: skip un-portable hack on windows + numpy2) #16589

Merged

pllim added the backport-v6.1.x label Jun 21, 2024

saimn reviewed Jun 25, 2024

View reviewed changes

neutrinoceros mentioned this pull request Jun 25, 2024

UX: warn about any (broken) leftover references to closed memory map objects #16608

Merged

1 task

pkgw mentioned this pull request Jul 22, 2024

Uncaptured requirement of Numpy < 2 on Windows conda-forge/astropy-feedstock#140

Closed

1 task done

This was referenced Jul 24, 2024

TST: Windows job has 4 failures #16765

Closed

TST: add a filter for unactionable warning on windows+numpy2 #16766

Merged

		@@ -0,0 +1 @@
		Skip an un-portable hack for Windows if NumPy 2.0 or newer is installed.

		This fixes a crash that occurred for files oopened via
		``fits.open(..., mode='update')``.

Conversation

neutrinoceros commented Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions Bot commented Jun 17, 2024

Uh oh!

github-actions Bot commented Jun 17, 2024

Uh oh!

neutrinoceros commented Jun 17, 2024

Uh oh!

astrofrog left a comment

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Jun 17, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk commented Jun 17, 2024

Uh oh!

neutrinoceros commented Jun 17, 2024

Uh oh!

neutrinoceros commented Jun 17, 2024

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!

neutrinoceros commented Jun 18, 2024

Uh oh!

mhvk commented Jun 18, 2024

Uh oh!

mhvk commented Jun 18, 2024

Uh oh!

neutrinoceros commented Jun 18, 2024

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!

mhvk commented Jun 18, 2024

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!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

neutrinoceros commented Jun 17, 2024 •

edited

Loading