BUG: Allow empty memmaps in most situations

[laarohi]

• Fixes BUG: numpy._core.memmap Error when offset is a multiple of allocation granularity #27722
• Added a test case to confirm the fix.

[seberg]

This needs more explanation, I think. To me it also looks like this will simply fail if you pass offset=0. Even if I am missing something, that means it needs testing.
I.e. it looks a bit like it fixes your specific issue, but may not improve things generally.

It seems like a 0 size memmap is quite fraught to begin with, so I am not sure what is right. Setting the bytes to be at least 1 may make sense, but on linux may also fail if the size is actually 0 it seems (so at the very least it would also lead to a regressions in some places).
Of course sure, if an offset is used, that offset can be adjusted so that we don't need to "read" a byte beyond the end.

EDIT: Ack, sorry I guess the start > 0 prevents things, but it still would be nice to be clear that this only works with offsets. And it would be good to think about what to do about the other case.

[laarohi]

Agreed, I could have been more clear.

So just to clarify, on Linux (haven't tested on Windows), in the current numpy implementation:

• Works:
  • 0 size memmap with offset > 0 and offset % mmap.ALLOCATIONGRANULARITY != 0
• Fails:
  • 0 size memmap with offset = 0
  • 0 size memmap with offset > 0 and offset % mmap.ALLOCATIONGRANULARITY == 0

With the suggested fix:

• Works:
  • 0 size memmap with offset > 0
• Fails:
  • 0 size memmap with offset = 0

I agree that a 0 size memmap is quite fraught, but it would be nice if the behaviour of a 0 size memmap was independent of whether the offset was a multiple of the allocation granularity or not.

An alternative solution would be to not allow 0 size memmaps at all.

I do see some logic as to why it makes sense to allow a 0 size memmap with offset > 0 as this offset typically implies that the file contains some sort of metadata besides for the 0 size array. For a 0 size memmap with offset = 0 the file would be completely empty and thus there is nothing to map at all (mmap.mmap with length=0 even fails on Windows for an empty file (see here)).

[seberg]

Yeay, thanks. I have two asks:

1. Please add a test for arr.offset being correct, even though you are messing around with the start. (I suspect it already is.)
2. Can you add (ideally not too long) code comment about bytes=0 being problematic but we can avoid it easily if offset!=0. It can reference to this PR gh-27723.

But, maybe we can rethink the bytes=0 part as well? It seems to me that it is impossible to memory map an empty file on all systems. In other words, mapping 0 bytes works on linux probably, but only because it maps the full file and then reads nothing.

I think that means that we can:

• Use bytes=1 if offset=0
• Use your trick here if offset > 0 in order to not grow the file unnecessarily. Although, I don't think it would matter too much, that seems pretty safe to me.

Or am I missing some path where if bytes == 0 and offset == 0: bytes = 1 might backfire?

[laarohi]

I've added a test for arr.offset == offset, please confirm that this is what you had in mind as the test seems a bit trivial to me (but it could very well be that I misunderstood what you were asking).

I've also added a code comment as asked.

I also had a think about the 0 size memmap with offset=0 and your suggestion to use bytes=1 makes sense. I've implemented it in such a way that it requires mode in ('w+', 'r+') since we will be writing a byte to the file. There was an existing test for an empty memmap which I have adjusted accordingly.

Have a look at the changes and let me know what you think.

[seberg]

Thanks, i think this is ready, unless anyone is worried about padding empty files with a single byte to make almost all zero-size memmaps works.

@laarohi if you feel like this, I think it would make sense to add a brief enhancement release note (just a bullet point even) that empty memmaps are now better supported.
(They go into docs/release/upcoming_changes there is a README.md with some notes.

[laarohi]

@seberg enhancement release note added.

[seberg]

Thanks for the followups especially, @laarohi! Let's give this a shot. Is a backport important to you? Otherwise, I am not sure if I feel this is a change that really needs it.

[laarohi]

No need for a backport. Thanks for reviewing & merging @seberg !