Repository navigation
Work around memory errors when using memory mapping on large files in io.fits - #7926
Conversation
…only' fails. Specifically, mode='readonly' results in the memory-mapping using the ACCESS_COPY mode in mmap so that users can modify arrays. However, on some systems, the OS raises a '[Errno 12] Cannot allocate memory' OSError if the address space is smaller than the file. The solution is to open the file in mode='denywrite', which at least allows the file to be opened even if the resulting arrays will be truly read-only.
|
Hi there @astrofrog 👋 - 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 labeled 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! 👍 If there are any issues with this message, please report them here. |
|
The test we added has an issue with a non-closed file: if len(not_closed):
msg = ['File(s) not closed:']
for name in not_closed:
msg.append(' {0}'.format(name))
> raise AssertionError('\n'.join(msg))
E AssertionError: File(s) not closed:
E /Users/tom/Dropbox/Code/Astropy/astropy/astropy/io/fits/tests/data/test0.fits
/Users/tom/miniconda3/envs/dev/lib/python3.7/site-packages/pytest_openfiles/plugin.py:108: AssertionErrorHowever we just can't figure out what this is due to so we've added a commit to ignore this error as it is hopefully unimportant. @drdavella @saimn - if you have any ideas on what could have been causing that issue, please let us know! |
| return mmap_original(*args, **kwargs) | ||
|
|
||
| with fits.open(self.data('test0.fits'), memmap=True) as hdulist: | ||
| with patch.object(mmap, 'mmap', side_effect=mmap_patched) as p: |
There was a problem hiding this comment.
Might be completely off-base here, and I'm not sure whether this will fix the open files issue or not, but it seems like the order of these context handlers might need to be reversed?
There was a problem hiding this comment.
We've already tried every permutation, and it doesn't help
There was a problem hiding this comment.
After looking at this for a little while myself, I think I'm willing to say that this is probably just due to some weird interaction between mock and mmap, and so skipping the open files test here is okay.
|
The idea seems fine. In #1380 |
|
No I didn't see a ResourceWarning when testing it. |
|
@astrofrog - Yes, looks good |
|
Thanks @astrofrog, I guess we close #1380, as the other option |
|
@saimn - yes, sounds good! |
This is a workaround for #1380, which @Cadair and I ran into while trying to memory map a 30Gb file. The issue is that the available memory address space is less than the size of the file, the OS can in some cases raise a '[Errno 12] Cannot allocate memory' OSError even if it's not actually allocating all the memory (it's just testing if it would work preemptively).
Specifically,
mode='readonly'results in the memory-mapping using the ACCESS_COPY mode in mmap so that users can modify arrays (without writing the values back to disk). The solution as described in #1380 is to open the file inmode='denywrite', which at least allows the file to be opened even if the resulting arrays will be truly read-only.The change here is to modify
readarrayso that if this specific memory error occurs, it will fall back to reading the arrays as read-only arrays. This also will show a warning so that the user knows what's happening. There is no other way to access the data anyway in this case, so better to have read-only than not at all.This change should be backward-compatible, as it won't affect anything for cases that worked already. It simply means that some cases that crashed before won't anymore.
Of course, this is io.fits, so who knows
¯\_(ツ)_/¯- let's see what the CI says.I'm not sure if this is a bug fix as such, so tentatively milestoning for 3.1, but let me know what you think.
also cc-ing @keflavich and @embray (because of #1380)