Change DataInfo._parent to a weakref - #6277

This looks OK, though I'm still a bit confused by this; I thought simple reference cycles like col <-> info were found easily. Is it the column that is the problem, or the table?

@mhvk - the problem does not appear for just an isolated Column, only when it is within a Table. I am with you in not fully understanding this, but the fix clearly works...

I'm trying to investigate a bit more. Just wondering where the _parentis held...

I think I found it: it seems DataInfo needs a __delete__ method that removes instance.__dict__['info']._parent

Actually, what I found only ensures that del a.info does what it is supposed to do, but it doesn't help the real problem: that a column stays around until cyclic garbage collection; so, this PR is definitely better.

Am still wondering if it wouldn't be possible to do without the _parent at all - i.e., whether we can avoid storing _parent in instance.__dict__['info']...

Am still wondering if it wouldn't be possible to do without the _parent at all - i.e., whether we can avoid storing _parent in instance.dict['info']...

You need to store the instance somewhere (for other methods to use) in a way that is uniquely associated with the instance. There are other strategies, e.g. in a dict that is hashed by instance, but that requires that the instance be hashable. At the end I think you would basically be doing the job of a weak ref. That's my guess anyway, but I could be wrong.

Rebased.

I think you really only need to store _attrs - any time you access info, you go via __get__ and thus get passed the instance. Am still trying...

@mhvk - it would be good to get this (or an alternate) merged today, the plan is to cut the RC tomorrow (morning in Europe). Any thoughts?

Oops didn't see your alternate PR earlier. Will look.

I've tried my alternative further and I think it ends up being rather fragile; indeed, I'll close it as I think it is not going in the right way, and I don't have time to experiment further.

Hmm, I'm getting less happy with my version now too...

In [11]: col = Column([1,2,3], name='a', format='%d')

In [12]: col[:].info
Out[12]: --------------------------------------------------------------------------
AttributeError: 'NoneType' object has no attribute 'info'

Originally I thought it was just for something that requires the parent. It isn't obvious that this would be a common thing to do, but still it is a bit annoying.

Yes, that is annoying... Maybe, we need something in between the two approaches, where __dict__['info'] contains the DataInfo instance without _parent, or, in other words, were __get__ takes that instance and tags _parent on in a way that does not propagate back (some form of shallow copy?) But everything else does have to propagate back... I'll try to think a bit more tonight...

p.s. But just to be sure: I think this is still an improvement over being able to run out of memory with relatively trivial table operations!

Well I have improved this version somewhat, so there is no internal API change (made self._parent a property that resolves the weakref if necessary), and added a long exception error message that explains the workaround.

Maybe that's enough for now and we can always come back to it. We can rightly say that the lost parent situation is a bug and fix later.

And I just didn't want to use the technically-correct term "dead" parent here. It's just lost...

I've added the no-changelog-entry-needed as the release script recognizes that it isn't in the changelog (but it's not cleaver enough to realize it's not in the changelog as #6342 got it removed).

Will this be backported to 2.x? I think this is the cause of astropy/halotools#917

What troubles me is the memory management is not deterministic without this patch.

Here is the output of objgraph in a loop that overwrites a table repeatedly:

Graph written to /tmp/objgraph-tafljk13.dot (395 nodes)
dot: graph is too large for cairo-renderer bitmaps. Scaling by 0.485272 to fit
Image generated as cycles-000.png
219056
================================================================
Type              Old_ids  Current_ids      New_ids Count_Deltas
================================================================
dict                22262        22456         +194         +194
list                15890        15999         +109         +109
OrderedDict           356          451          +95          +95
ColumnInfo            297          391          +94          +94
Column                296          390          +94          +94
weakref              7688         7691           +3           +3
slice                 265          267           +2           +2
TableColumns            6            8           +2           +2
set                  6426         6427           +1           +1
function            32318        32319           +1           +1
================================================================

And a portion of the reference graph that shows the problem:

@rainwoodman , this was milestoned for 2.0.0, so technically it should already be in 2.x.

I commented on the wrong ticket. I meant the reinstating of the patch -- I suppose this was never reverted in 2.x then?

Indeed it wasn't. Sorry for the noise.

Indeed, #7448 hasn't been backported yet, but planned to be included in 2.0.8.

@astrofrog @eteq - shall we just create an "invalid" or similar milestone that reverted PRs like this and #6342 can go into?

@rainwoodman - I'm sorry, the decision was at the end to not include this in the LTS branch given the API change.
The fix will be though in 3.0.4 released later today.

@rainwoodman - to add a bit more here: is there a specific reason this was important for the LTS vs 3.0.x/master? Here we're in the awkward position of it being both a bug and an API change required... which we try to avoid wherever possible on LTS'.

but if there's a specific compelling reason/use case for it to go back to the LTS that could be reconsidered for 2.0.9 ...

There doesn't seem to be any API breakage by looking at this patch.

There is a _parent attribute before and after the change. Only in already undefined situations, accessing _parent attribute may fail after this change. Also, _parent is private anyways.

The API break is quite obscure, but previously col[2:4].info.descr would work, but with this patch it can fail since the parent goes out of scope before the descr is fetched. Now you have to do:

tmp = col[2:4]
tmp.info.descr

Look like there is no easy fix then. Is this also a problem in astropy3?

Looks like this DataInfo is a blend of two interfaces -- storing the attributes and deriving properties of the column. The derived property interface requires a reference from info to parent. The storing interface requires a refrence from parent to info.

If they are split then there is no longer a need for cyclic reference. The storing interface could have been merged into the column itself or as something fancier, but it doesn't need to know the parent. The interface for derived properties can be done by always creating a new instance from the parent object upon access, as it doesn't involve storing anything the creation is fast enough -- one can still provide access to the stored attributes here to maintain compatibility, I think.