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

memmap refactoring #1265

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

samuelgarcia
Copy link
Contributor

This is a proposal to change the actual memap behavior in rawio layer.

spikeinterface use neo.rawio a lot with extenssive IO demand with parralel read/computing.
Many users experience big memmory saturation even on big machine when the file are enormous.
This quite a bit OS dependant, mainly windows but even some linux have this problem).
Somehow, the memmap do not release page after the use of one buffer and so the memory increase forever.

The actual strategy we now have in almost all IO when it is binary base :

  1. In parse_header, open one or several numpy.memmap object and set then as attribute or nested dict
  2. In get_analogsignal_chunk : lazly fetch buffer via numpy slicing using this memmap arrays
    After consumming the traces chunk the memory should be relased and so the memmory usage should low but in fact in some case it increase until saturating.

@h-mayorquin and @samuelgarcia made some benchmark to find a better strategy.
Here some code examples:
https://gist.github.com/h-mayorquin/8b8d1899c724690ce7c18fb49f56af82

The best strategy we find to try to fix is:

  1. In the parse header, open the file with mode='rb'
  2. In get_analogsignal_chunk : create a new memmap object using the opened file

For the 2. we mimic the numpy.memmap but a but simplify to be faster.
The speed of reading is almost the same and the memory problem seems to be fixed.

@maxjuv @TomBugnon @vncntprvst @juliencarponcy: we need more tests!
I started to change spikeglx and openephysrawbinary.
Could you try to combine this branch and also this PR on spikeinterface
And then:

  • read a recording with read_spikeglx() or read_openephys()
  • make a simple preprocessing
  • rec.save(folder=folder, n_jobs=64, chunk_duration='1s', progress_bar=True)
    And monitor the memory usage.
    Do you still have an infinite memory increase or not ?

@zm711
Copy link
Contributor

zm711 commented May 5, 2023

Hey Sam, just as an fyi and to support the need for this refactor I was doing some emg processing (outside of the spike sorting world) and saw the same thing where I import neo in a function create a reader and then return the analog emg data for post-processing and it seems that as long as my IPython kernel is active the memmap and header information aren't releasing (ie, saturating the RAM eventually). Despite the reader being "cleaned up" after the function if I call the same function on the binary file it runs significantly faster (seems like the header and memmap have been cached). Closing the session finally releases the RAM. On a windows for this work currently (which does annoyingly hold onto files way longer than Mac and Linux do).

@samuelgarcia
Copy link
Contributor Author

@zm711 thanks for the feedback. So do you this this patch will help your case ?

@zm711
Copy link
Contributor

zm711 commented May 5, 2023

@samuelgarcia
I'm using the IntanRawIO, so I can't specifically test this, and this issue only occurs when I'm trying to batch process data, so it's a rarer issue for me, (so I never reported it I just dealt with my computer crashing every once in a while). If this test actually works for the more extreme spikeinterface case, then I would predict it would also be helpful in the edge cases of data analysis I run into. But I would just need to actually see if this is a general windows issue, ipython issue, or np.memmap issue.

@jpgill86
Copy link
Contributor

jpgill86 commented May 5, 2023

@zm711, I wonder if your memory release issue is related to #684. Manual garbage collection might help:

import gc
gc.collect()

@zm711
Copy link
Contributor

zm711 commented May 5, 2023

@jpgill86

I tried the gc.collect() and the memory usage didn't change. I tried to two rounds of experiments with the first gc.collect() returning 23 unreachable objects and the second round returning 86.

I do have some try except blocks for some of this analysis, so the main memory lock occurs if parts of the meta-function fails and moves onto trying a different sub-function due to needing something like #1249 for gaining digital access in addition to analog access (which again is IntanRawIO specific).

The memory getting locked up is a bit inconsistent so it's hard for me to know for sure what is causing it and it is rare enough that hunting it down isn't hugely important to us. So I'm happy to try new implementations to see if anything helps, but I don't want to bog down this PR with memory leak stuff so if we want to continue this we could chat on #684?

@maxjuv
Copy link

maxjuv commented May 9, 2023

Hello,
I tried theses Neo & Spikeinterface branches on my Windows. While the memory used is a bit high (7G) for 14 cores, 1s chunksize and the processing consisted to highpass/phase shift/ detect_bad channels/CMR on a Neuropixel dataset. However, the memory stays stable at 7G, so it is way better than before.

@JuliaSprenger
Copy link
Member

Hi @samuelgarcia The tests in the current master are passing again. You might need to rebase / merge for proper CI testing.

@TomBugnon
Copy link

Hi @samuelgarcia
Sorry for the late reply.

I did not check the memory usage (which I didn't specifically have issues with), but relative to the parent commit (2d63e18) this fixes a OSError: cannot allocate memory I'm getting when too many memmaps/recordings are opened relying on the same file!

For instance this would raise the following error:

import spikeinterface.extractors as se
path = "/path/to/sglx"
extractors = []
for i in range(50):
    extr = se.SpikeGLXRecordingExtractor(, stream_id="imec0.ap")
    extractors.append(extr)

Traceback (most recent call last):
File "/home/tbugnon/projects/ecephys_dev_si_pipeline/pipeline_tests/test_cannot_allocate_memory_error.py", line 49, in
extr = se.SpikeGLXRecordingExtractor("/Volumes/ceph-tononi/npx_archive/CNPIX2-Segundo/1-21-2020/SpikeGLX/1-21-2020_g0/1-21-2020_g0_imec0", stream_id="imec0.ap")
File "/home/tbugnon/projects/ecephys_dev_si_pipeline/spikeinterface/src/spikeinterface/extractors/neoextractors/spikeglx.py", line 51, in init
NeoBaseRecordingExtractor.init(
File "/home/tbugnon/projects/ecephys_dev_si_pipeline/spikeinterface/src/spikeinterface/extractors/neoextractors/neobaseextractor.py", line 94, in init
_NeoBaseExtractor.init(self, block_index, **neo_kwargs)
File "/home/tbugnon/projects/ecephys_dev_si_pipeline/spikeinterface/src/spikeinterface/extractors/neoextractors/neobaseextractor.py", line 48, in init
self.neo_reader = get_neo_io_reader(self.NeoRawIOClass, **neo_kwargs)
File "/home/tbugnon/projects/ecephys_dev_si_pipeline/spikeinterface/src/spikeinterface/extractors/neoextractors/neobaseextractor.py", line 39, in get_neo_io_reader
neo_reader.parse_header()
File "/home/tbugnon/projects/ecephys_dev_si_pipeline/python-neo/neo/rawio/baserawio.py", line 178, in parse_header
self._parse_header()
File "/home/tbugnon/projects/ecephys_dev_si_pipeline/python-neo/neo/rawio/spikeglxrawio.py", line 103, in _parse_header
data = np.memmap(info['bin_file'], dtype='int16', mode='r', offset=0, order='C')
File "/home/tbugnon/miniconda3/envs/ecephys_dev_si_pipeline/lib/python3.10/site-packages/numpy/core/memmap.py", line 267, in new
mm = mmap.mmap(fid.fileno(), bytes, access=acc, offset=start)
OSError: [Errno 12] Cannot allocate memory

And is fixed by this PR.
This happens for our very large recordings. I was NOT able to replicate it on the short recordings from spikeinterface.core.datasets (maybe a size dependence of the number of memmaps?)

I did not have to check out PR 1602 on spikeinterface for this behavior.

Let me know if I can help somehow! Looking forward to seeing it merged
Best
Tom

@samuelgarcia
Copy link
Contributor Author

Salut Tom.
Merci pour le feedback.
I will try to finalize this soon.

@JuliaSprenger JuliaSprenger added this to the 0.13.0 milestone Jun 14, 2023
@TomBugnon
Copy link

Hi @samuelgarcia
do you have any ETA regarding this PR? And/or is this branch safe to use as such or does it need more testing?

memmap = self._memmaps[seg_index, stream_id]
#~ memmap = self._memmaps[seg_index, stream_id]
key = (seg_index, stream_id)
memmap = create_memmap_buffer(*self._memmap_args[key])
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you use get_memmap_shape to avoid creating a buffer here?

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.

9 participants