Skip to content

BUG: skip un-portable hack on windows + numpy2 #16581

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mhvk merged 2 commits into from
Jun 18, 2024
+9 −3
Merged

BUG: skip un-portable hack on windows + numpy2 #16581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2be96df
BUG: skip un-portable hack on windows + numpy2
neutrinoceros Jun 17, 2024
239c9d4
a (possibly) clearer changelog
neutrinoceros Jun 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion astropy/io/fits/hdu/hdulist.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
)
from astropy.io.fits.verify import VerifyError, VerifyWarning, _ErrList, _Verify
from astropy.utils import indent
from astropy.utils.compat.numpycompat import NUMPY_LT_2_0

# NOTE: Python can be built without bz2.
from astropy.utils.compat.optional_deps import HAS_BZ2
Expand Down Expand Up @@ -1506,7 +1507,7 @@ def _flush_resize(self):
del hdu.data
hdu._file = ffo

if sys.platform.startswith("win"):
if sys.platform.startswith("win") and NUMPY_LT_2_0:
# On Windows, all the original data mmaps were closed above.
# However, it's possible that the user still has references to
# the old data which would no longer work (possibly even cause
Expand All @@ -1517,6 +1518,8 @@ def _flush_resize(self):
# lead to odd behavior in practice. Better to just not keep
# references to data from files that had to be resized upon
# 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:

@saimn saimn Jun 25, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@neutrinoceros neutrinoceros Jun 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@neutrinoceros neutrinoceros Jun 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if mmap is not None:
# https://github.com/numpy/numpy/issues/8628
Expand Down
5 changes: 5 additions & 0 deletions docs/changes/io.fits/16581.bugfix.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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

@mhvk mhvk Jun 18, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@eerovaher eerovaher Jun 18, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for downstream code run under these exact conditions. Please report any
regression found.
2 changes: 0 additions & 2 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,6 @@ keywords = [
]
dependencies = [
"numpy>=1.23",
# https://github.com/astropy/astropy/issues/16578
"numpy < 2.0 ; platform_system=='Windows'",
"pyerfa>=2.0.1.1",
"astropy-iers-data>=0.2024.5.27.0.30.8",
"PyYAML>=3.13",
Expand Down