Use mmap_mode for probing

[nh2]

Fixes plyfile incorrectly falling back to non-mmap read even when mmap is supported.

Until now, for example, when the user wanted a read-only ("r") mmap and that would succeed, plyfile's _can_mmap() check would not use "r" but "c" for the check, which would fail for large files.

That in turn would then cause OOMs, because with _can_mmap() == False it would fall back to normal full reading.

---

📚 Documentation preview 📚: https://python-plyfile--86.org.readthedocs.build/en/86/

[dranjan]

Hey @nh2, thanks for the patch! This looks pretty simple, so it shouldn't take long to review and merge.

By the way, the documentation preview action failed because your branch was made from an earlier release, and the latest commits on master are needed for readthedocs.org to do the build. You don't need to do anything about this.

[dranjan]

The changes make sense to me.

Besides my one comment about the docstring, the only other thing I can think of at the moment is that it would be nice to add a test for this bug fix, but I can't immediately think of how. I guess we would need to make a file object that's readable and memory-mappable but can't be memory-mapped in copy-on-write mode, but if we need to trigger the memory overcommitment issue, I don't think we can do that in a unit test. If you can think of a solution, do let me know. However, to be clear, this isn't blocking the pull request.

If you want to try to address the docstring thing, let me know. Otherwise, I'll probably make the changes and merge later this week and then push a point release.

[dranjan]

I don't think this information belongs in the docstring. If anywhere, it should be in a comment near the call site, and probably summarized a bit.

[nh2]

Happy to move it anywhere, but I'm curious why it wouldn't belong into the docstring. Isn't this info that any caller must know to use the function correctly?

[dranjan]

This function simply checks if the stream can be mapped in the requested mode, and that's all the caller should need to know. That text is talking about the probing and the subsequent mapping and the relationship between the two, which seems a bit out of scope in that docstring.

As for where the info should go...actually after this bug fix, I think the code just makes sense and needs no further explanation, so even a comment probably isn't needed. However, the reason for the bug fix is interesting, so I'm thinking we put it in the change log.

Since I'm clearly having trouble making up my mind, I don't want to waste more of your time with this, so I'll just make the decision myself and merge it within a few days.

[nh2]

Fair enough, I kicked the comment.

--

Side question:

Why does it say mmap : {'c', 'r', 'r+'} or bool in def _read? The mmap_mode = mmap if isinstance(mmap, str) else 'c' looks like it's use 'c' even for mmap=False.

[dranjan]

That whole block is under an if mmap and ... clause, so mmap is already a truthy value by that point.

[nh2]

Ah fair.

[nh2]

@dranjan Bringing this back up from another perspective: I noticed:

plyfile.PlyData.read(source_ply_path, mmap=True) results in a 'c' mapping, so it may fail if there's not enough memory for writing to it. So I'm not sure if the boolean mmap is a great idea. Given that the function is called .read(..., mmap=True) it's a bit surprising that that wouldn't use 'r', isn't it?

This oddity dates back to the original introduction of mmap usage in python-plyfile:
8fa111b#diff-57deaf044370d3ad76437036130da4a60a4ea697b80de5fb145ed01eea0179feR583

The mmap bool was introduced in:
nh2@260999b

Note that in our code, too, uses .read(..., mmap='r+') to make a writeable handle even though it goes through the function called .read(). Maybe that function should really be called .open(). In any case, would it make sense to have mmap=True to default to mean mmap='r' for something called .read()`.

However, I feel like changing the meaing of True -> 'c' to True -> 'r' would break users' code at runtime.
Probably nothing can be done about this, given it's enshrined in history now.

So I think that in the existing docs

        mmap : {'c', 'r', 'r+'} or bool, optional (default='c')
            Configures memory-mapping. Any falsy value disables
            memory mapping, and any non-string truthy value is
            equivalent to 'c', for copy-on-write mapping.

should be extended to explain that 'c', for copy-on-write mapping really isn't a good default if you just want to .read(), and you should use 'r' explicitly, because otherwise you might OOM surprisingly.

[dranjan]

You're right, it should be documented. However, I still think that mmap='c' does the most reasonable thing in the largest number of cases, so it should be the default. From an API perspective, the only viable defaults are mmap=False and mmap='c', the reason stemming from the fact that not all PLY elements are memory-mappable. We don't want to require the user to be aware of memory mapping just to use the package, which means the default behavior should behave functionally like a non-memory-mapped file. Because the API has always supported modifying PLY element data in place, the only two mmap values with the right behavior are False and 'c', and of those two, I think mmap='c' is strictly better in almost all cases, since you get a performance boost at least some of the time, and you can also potentially open larger files.

I hope that also clarifies why the function is called read and not open, because reading is the core functionality. mmap='r+' is really for expert users and not supposed to be the primary behavior, and in fact, it's barely documented at all. Consider that there's a huge difference between non-memory-mapped elements and memory-mapped elements with this option, so the user must be acutely aware of the distinction. (The story with mmap='r' is similar, just much less catastrophic if ignored.)

[dranjan]

I added a short note to the documentation here: https://python-plyfile.readthedocs.io/en/latest/usage.html#memory-mapping-very-large-files I'll probably cut the release tomorrow or so.

[nh2]

the only viable defaults are mmap=False and mmap='c', the reason stemming from the fact that not all PLY elements are memory-mappable

Ah, I see now. When you mmap='r', you can no longer e.g. numpy-assign into the resulting array, and that is weird, because you can do that with mmap=False.

Your added docs make it clear, thank you!

[nh2]

but if we need to trigger the memory overcommitment issue, I don't think we can do that in a unit test

I agree; I tested it locally by just mapping a PLY file that's larger than my memory, but the exact behaviour of that depends on the overcommit settings of the machine, the amount of RAM, and Linux's heuristics if the vm.overcommit_memory setting is 0. So I don't think we can make a test for it that won't be at least a little bit dependent on the system of whoever runs it.