Reviving #6277 that was merged but then reverted - #7448

Hi there @pllim 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

I noticed the following issue with this pull request:

• Changelog entry section (v3.1) inconsistent with milestone (v2.0.7)

Would it be possible to fix this? Thanks!

If there are any issues with this message, please report them here.

@pllim @astrofrog - What about backporting here?

Let me know if you want to backport and I'll move the change logs. I am just opening this up so we can push things forward, so I don't have strong opinions either way.

I would say let's backport but only once this has been merged into master for a while and has shown to be stable.

@pllim - could you preserve the original commits from #6277? Or maybe give a more explicit commit message that actually describes the fix so that it's clear what this does without having to go to GitHub.

@cdeil - could you try out this branch to see if the issues you were having with the open files goes away?

@astrofrog - OK, I keep mind to hold off backport, but still remilestoned otherwise it will slip though.

@bsipocz , do I have to move the change logs?

@astrofrog , I have rewritten the commit message as you requested.

I guess the changelog depends on if we will backport

@pllim - Don't worry about the changelog, we merge this as is and I'll put it in the right place if/after backported (actually it's even better not to put in 2.0.7 now, so there'll be more red flags raised that will remind me not to backport straight away...).

All the CIs passed. The only "x" is from bot because of the change log. FYI.

@cdeil - could you try out this branch to see if the issues you were having with the open files goes away?

I tried the example from #7440 (comment) here and from what I see this PR does fix #7447, i.e. the table / column is reliably deleted, no more memory leak, and on delete the memmap is closed.

But technically no, this change alone would not resolve our issue in Gammapy with open files, because we actually need to hold on to the data. But I think that memmap question is irrelevant for the discussion / fix here, no?

@cdeil - thanks for confirming!

This is on my radar but I was away on vacation last week. I need to refresh on the whole long discussion.

One question, probably for @astrofrog - do you believe that #6277 was exonerated as the source of the "Heisenbug"?

Maybe #6703 / #6714 was related to this bug ?
If everything works here we can probably merge and see if the bug appear again.

@taldcroft - well I can't guarantee it, but my past self apparently uncovered that the heisenbug appeared shortly before #6277 was merged, so I think so. To some extent there's only one way to find out 😬

Fine by me to merge.

@pllim - can you move the changelog entry prior to merging? (you can use [ci skip])

@astrofrog - Let's keep the changelog for now to make sure this PR generates an anomaly for the backports (#7448 (comment))

2 approvals above, so merging this now. Thanks @pllim for the resurrection of the fix.

I feel we have enough testing for this in master, so as this indeed seems to be working I'm going to backport it to the bugfix branches.

@taldcroft @pllim @astrofrog - Is it OK for this API change to get backported? If yes, 3.0.x only or LTS, too?

I'm not adamant, but I would say that 3.0.x is OK but not LTS. Somehow I feel like LTS is more sacred when it comes to any API change, even it truly obscure.