openslide / openslide-python

Python bindings to OpenSlide
https://openslide.org/
GNU Lesser General Public License v2.1
374 stars 183 forks source link

openslide memory leak #24

Open gabricampanella opened 7 years ago

gabricampanella commented 7 years ago

Context

Issue type (bug report): Operating system (CentOS 7.3.1611): Platform (x86_64 GNU/Linux): OpenSlide Python version (1.1.0): Slide format (SVS):

Details

Dear openslide team, Thank you for making this software available. It greatly facilitates my research. I am building large-scale pipelines for digital pathology that rely on extracting tiles from thousands of slides on the fly. To achieve this I create all openslide objects at the beginning and append them to a list. I fetch tiles by looping through a list of pixel coordinates and slide index. The loop somehow leaks memory. The memory used keeps increasing until out of memory. The script below reproduces this behavior. You only need to change the path to a slide to make it work.

import openslide
import random
import sys
import numpy as np

class library(object):
    def __init__(self, slides, idx, grid, size):
        sl = []
        for i,slide in enumerate(slides):
            sl.append(openslide.OpenSlide(slide))
            sys.stdout.write("{}/{} \r".format(i+1,len(slides)))
            sys.stdout.flush()
        tup = zip(grid,idx)
        self.size = size
        self.slides = slides
        self.tup = tup
        self.sl = sl

    def make_list_of_objs(self):
        sl = []
        for i,slide in enumerate(self.slides):
            sl.append(openslide.OpenSlide(slide))
            sys.stdout.write("{}/{} \r".format(i+1,len(self.slides)))
            sys.stdout.flush()
        self.sl = sl

    def __getitem__(self, index):
        tup = self.tup[index]
        img = self.sl[tup[1]].read_region(tup[0],0,(self.size,self.size)).convert('RGB')
        return img

    def __len__(self):
        return len(self.tup)

class batch_loader(object):
    def __init__(self,dset,n):
        batches = []
        for i in range(0,dset.__len__(),n):
            d = min(n,dset.__len__()-i)
            batches.append(range(i,i+d))
        self.batches = batches

#MAKE DATA
slides = ['/scratch/gabriele/prostate_dict/389973.svs']*2000 #CHANGE SLIDE NAME
idx = list(np.random.choice(len(slides),2000000,replace=True))
gridx = list(np.random.choice(10000,2000000,replace=True))
gridy = list(np.random.choice(10000,2000000,replace=True))
grid = zip(gridx,gridy)

#SET UP LOADER
dset = library(slides,idx,grid,224)
loader = batch_loader(dset,512)

#LEAKING LOOP
for ii,batch in enumerate(loader.batches):
    imgs = []
    for i,idx in enumerate(batch):
        img = np.array(dset.__getitem__(idx))
        imgs.append(img)
    sys.stdout.write("{}/{} \r".format(ii+1,len(loader.batches)))
    sys.stdout.flush()

To circumvent this one can delete the list of openslide objects and recreate it every few iterations. This stabilizes memory footprint but it is an unfortunate work-around that wastes a lot of time reopening openslide objects:

import openslide
import random
import sys
import numpy as np

class library(object):
    def __init__(self, slides, idx, grid, size):
        sl = []
        for i,slide in enumerate(slides):
            sl.append(openslide.OpenSlide(slide))
            sys.stdout.write("{}/{} \r".format(i+1,len(slides)))
            sys.stdout.flush()
        tup = zip(grid,idx)
        self.size = size
        self.slides = slides
        self.tup = tup
        self.sl = sl
        self.idx = 0

    def make_list_of_objs(self):
        sl = []
        for i,slide in enumerate(self.slides):
            sl.append(openslide.OpenSlide(slide))
            sys.stdout.write("{}/{} \r".format(i+1,len(self.slides)))
            sys.stdout.flush()
        self.sl = sl

    def __getitem__(self, index):
        if self.idx > 5120:
            self.idx = 0
            self.make_list_of_objs()
        self.idx += 1
        tup = self.tup[index]
        img = self.sl[tup[1]].read_region(tup[0],0,(self.size,self.size)).convert('RGB')
        return img

    def __len__(self):
        return len(self.tup)

class batch_loader(object):
    def __init__(self,dset,n):
        batches = []
        for i in range(0,dset.__len__(),n):
            d = min(n,dset.__len__()-i)
            batches.append(range(i,i+d))
        self.batches = batches

#MAKE DATA
slides = ['/scratch/gabriele/prostate_dict/389973.svs']*2000 #CHANGE SLIDE NAME
idx = list(np.random.choice(len(slides),2000000,replace=True))
gridx = list(np.random.choice(10000,2000000,replace=True))
gridy = list(np.random.choice(10000,2000000,replace=True))
grid = zip(gridx,gridy)

#SET UP LOADER
dset = library(slides,idx,grid,224)
loader = batch_loader(dset,512)

#LEAKING LOOP
for ii,batch in enumerate(loader.batches):
    imgs = []
    for i,idx in enumerate(batch):
        img = np.array(dset.__getitem__(idx))
        imgs.append(img)
    sys.stdout.write("{}/{} \r".format(ii+1,len(loader.batches)))
    sys.stdout.flush()

Thanks for your patience.

bgilbert commented 7 years ago

Hi @gabricampanella. Each OpenSlide object has an internal cache of decoded pixel data that can grow to 32 MiB. There's currently no API to configure the size of that cache (https://github.com/openslide/openslide/issues/38).

My usual recommendation is to keep a fixed-length cache of OpenSlide handles, closing old ones as necessary. That works reasonably well if the workload exhibits locality, such as in the backend of a web-based slide viewer, but of course it's not great for truly random workloads. Do you have any opportunity to coalesce multiple accesses to a slide?

gabricampanella commented 7 years ago

Thanks for your reply. If I understand correctly, if each handle can grow to 32 MiB then if I open 2000 of them, they should grow no more than 64 GiB. But I observe a memory footprint that goes well beyond that. I believe that in a supervised learning framework the fact that samples are drawn independently from each other is important. I think complete randomness when extracting tiles is more correct, but practically it could be that good learning is achieved also by having batches of tiles all coming from the same slide. For now I am more inclined to use the hack than to eliminate random sampling.

bgilbert commented 7 years ago

For what it's worth, you're also storing at least 373 GiB of pixel data in imgs.

If you have a fixed set of samples to collect, as in your reproduction code, you could do it in two passes: first pick a set of samples, then sort by source slide, extract the pixel data, and re-sort into the original order. That way you'd only need to have one slide open at a time.

gabricampanella commented 7 years ago

imgs should contain only 512 images of size 3x224x224 (In other words 77M floats). It gets rewritten at every step of the loop. It definitely shouldn't grow to 370GiB.

I see what you are saying. In that case If I am unlucky I would need to open 512 slides per batch (if the batch size is 512). In the best case scenario I only get to open 1. It is an interesting idea. I'll think about it.

bgilbert commented 7 years ago

Whoops, you're right about imgs, I misread.

happyalfred2016 commented 1 year ago

I have a similar issue. I am trying to load patches from slides in the training of deep learning model. But even though I closed the slides (OpenSlide.close()) after each patch loading, the memory still grow heavily. Do you have any suggestions or workaround?