-
Notifications
You must be signed in to change notification settings - Fork 25
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
Clear unused intermediate DataFrame memory in channel averaging function #559
Conversation
@ngreenwald can you see if this fixes your kernel dying error? UPDATE: if so, I'll see if there are any other places we can use explicit |
Yeah, won't get to it until this weekend, can you check if it resolves
memprofiler issues?
…On Fri, May 20, 2022 at 2:16 PM alex-l-kong ***@***.***> wrote:
@ngreenwald <https://github.com/ngreenwald> can you see if this fixes
your kernel dying error?
—
Reply to this email directly, view it on GitHub
<#559 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADJB47P7QNA6DR7TH3PEOUDVK76LDANCNFSM5WQTNQNQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@ngreenwald it does on my end. |
I closed the docker session, restarted, and it worked. So it seems like there's some leftover memory leaks from previous cells that are run which then causes the kernel to die when that one is run. Can you run profiling on the entire pixel clustering workflow to try and figure out what's causing it? |
Different version of trained SOM, this time it died on the first step, "using remapping scheme." This is even though I had just started up the docker. Given that all of these different versions of the clustering pipeline are using the same data, I don't understand the randomness of it sometimes dying and sometimes not |
Actually, this was after I switched to a different branch. So it seems like this change successfully addressed the issue underlying the remapping step, since that error cropped up again as soon as I switched, but not the overall memory issues |
@ngreenwald yeah I kind of suspected this might happen, which is why I alluded to needing to flush out the memory elsewhere using |
@ngreenwald identified The reason these are much larger is because we have to store both a Explicitly calling |
@ngreenwald the update should ensure large, intermediate DataFrames (especially during loops) are released. Tested it on my end without memory errors, can you see if it helps on your end? |
Crashed on the |
@ngreenwald yeah everything in One option potentially is to pre-compute this average in R instead while Another option I'm trying out right now is directly invoking the Python garbage collector using |
@ngreenwald that's good to know. Let me focus on memory profiling that function to see if there's anything popping up. There wasn't anything when I ran it on my end, but I'll double-check on your branch. |
This issue was never replicated outside my laptop |
What is the purpose of this PR?
Closes #548. Pixel channel averaging needs to be an iterative process due to the way they are batched per FOV. These individual pixel cluster files can be extremely large, and Python does not automatically release them from memory even if the variable is overwritten on the next iteration. This means that for massive datasets, Docker could easily run out of memory. This PR should prevent this from happening.
How did you implement your changes
compute_pixel_cluster_channel_avg
will need to explicitly flush out these large, unused DataFrames withdel
. We do this for the intermediatefov_pixel_data
,sum_by_cluster
,count_by_cluster
, andagg_results
variables. This is especially important for the middle 2, becausegroupby
objects can be very computationally expensive.Remaining issues
This may not be the only memory adjustment we need to make, but it's a start.