Skip to content
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.

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

Ensure memory-mapped files are safely closed #81

Closed
wants to merge 1 commit into from

Conversation

gmgunter
Copy link
Member

@gmgunter gmgunter commented Sep 11, 2024

Adds a new class io.MMapArray, an array_like type that uses a memory-mapped file buffer for storage, and replaces usage of numpy.memmap with io.MMapArray throughout the codebase.

numpy.memmap does not have an API that guarantees that the underlying file is closed:

Currently there is no API to close the underlying mmap. It is tricky to ensure the resource is actually closed, since it may be shared between different memmap instances.

This could lead to a large number of file descriptors staying open for much longer than necessary. It seems to have been the cause of errors encountered in @scottstanie's testing due to too many open files when unwrapping multiple interferograms in parallel.

OSError: [Errno 24] Too many open files

TODO: fix pre-commit failures, finish docstrings, add unit tests

@scottstanie will you please test this out and see if it resolves the issue for you?

* Add `io.MMapArray`, an array_like type that uses a memory-mapped file
  buffer for storage
* Replace usage of `numpy.memmap` with `io.MMapArray`

`numpy.memmap` does not have an API that guarantees that the underlying
file is closed. This could lead to a large number of file descriptors
staying open for much longer than necessary.

>Currently there is no API to close the underlying mmap. It is tricky to
>ensure the resource is actually closed, since it may be shared between
>different memmap instances.

https://numpy.org/doc/stable/reference/generated/numpy.memmap.html

In practice, this seems to have been the cause of errors encountered in
testing due to too many open files when unwrapping multiple
interferograms in parallel.

```
OSError: [Errno 24] Too many open files
```
@gmgunter gmgunter marked this pull request as draft September 11, 2024 08:20
Copy link

codecov bot commented Sep 11, 2024

Codecov Report

Attention: Patch coverage is 85.04673% with 16 lines in your changes missing coverage. Please review.

Project coverage is 91.97%. Comparing base (6b61e66) to head (b808ab0).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/snaphu/io/_mmap_array.py 82.92% 14 Missing ⚠️
src/snaphu/_unwrap.py 84.61% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #81      +/-   ##
==========================================
- Coverage   93.44%   91.97%   -1.48%     
==========================================
  Files           9       10       +1     
  Lines         473      548      +75     
==========================================
+ Hits          442      504      +62     
- Misses         31       44      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@scottstanie
Copy link
Contributor

not related to this PR, but i retried an editable install after checking out the branch, and it didn't error on install... but then I got this during the run

  File "/Users/staniewi/repos/snaphu-py/src/snaphu/_conncomp.py", line 307, in grow_conncomps
    regrow_conncomp_from_unw(
  File "/Users/staniewi/repos/snaphu-py/src/snaphu/_conncomp.py", line 110, in regrow_conncomp_from_unw
    run_snaphu(config_file)
  File "/Users/staniewi/repos/snaphu-py/src/snaphu/_snaphu.py", line 89, in run_snaphu
    subprocess.run(args, stderr=subprocess.PIPE, check=True, text=True)
  File "/Users/staniewi/miniconda3/envs/mapping-311/lib/python3.11/subprocess.py", line 548, in run
    with Popen(*popenargs, **kwargs) as process:
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/staniewi/miniconda3/envs/mapping-311/lib/python3.11/subprocess.py", line 1026, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/Users/staniewi/miniconda3/envs/mapping-311/lib/python3.11/subprocess.py", line 1950, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: '/Users/staniewi/repos/snaphu-py/src/snaphu/snaphu'

so I wonder if it's possible to support that for testing

@scottstanie
Copy link
Contributor

Unfortunately i dont think this is working (on Mac still).

I dont quite get why psutil doesnt show the memmaps that still have a file descriptor, but lsof is showing the list growing over time (screencast from running watch -n 0.3 "lsof -p 91685 | grep snaphu" while that dolphin process is running ):

snaphu-open-fds

Also just clarifying- this is not doing any parallel processing; it's just unwrapping each interferogram in serial

@bhawkins
Copy link

@scottstanie found this issue which will be fixed in the next Python release (v3.13) with the addition of the trackfd=False option:

https://docs.python.org/3.13/whatsnew/3.13.html#mmap

@gmgunter
Copy link
Member Author

Closing this in favor of #82

@gmgunter gmgunter closed this Sep 13, 2024
@gmgunter gmgunter deleted the memmap-closing branch September 13, 2024 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants