Skip to content

Table column print format cache bug #6703

Closed

Description

@larrybradley

The column format is currently cached in a global dictionary keyed with the format and id() of the column. The problem is that when the table/column get garbage collected, a totally different column can be later placed in the same memory location and thus have the same id(). That may seem unlikely, but I've been encountering this seemingly random bug in real code (and in photutils CI testing).

The specific issue that I'm having is that a mixin Quantity column caches the formatter to use the one for Quantity objects: https://github.com/astropy/astropy/blob/master/astropy/units/quantity.py#L121

Later, a regular non-mixin Column cannot be printed because it's trying to use the Quantity formatter. This is because the initial column has been garbage collected and the later Column has the same id() -- thus it uses the cached formatter.

This was frustrating to debug in actual code because it was hard to reliably reproduce, but here's a contrived example showing the bug:

https://gist.github.com/larrybradley/57185c001ae12647d74d9a73a53f6a5e

My suggestion would be to cache the formatter in the column info object, as suggested in #5804. I don't understand the concern about the case when the column does not yet have data. Perhaps I missed something, but it appears that the caching occurs only when the first item of the column is printed, so the column must have data in order for the format to be cached. Also, how does one even set the column format without having defined the column object?

Also related to: #5800 and #5803

CC: @taldcroft, @mhvk

Activity

  1. added this to the v2.0.3 milestone on Oct 10, 2017
  2. mhvk commented on Oct 10, 2017

    @mhvk
    Contributor

    @larrybradley - yikes, sorry to have had you debug that - I can see that that would have been far from trivial to find! I agree with your conclusion, that we should implement #5804. I should add that I'm not sure I'll have time soon to actually do that... I will add a comment to #5804 about how (inspired by what you write above).

  3. taldcroft commented on Oct 10, 2017

    @taldcroft
    Member

    I took a look at this during a plane flight and I might be able to tackle this in the next week or two.

  4. saimn commented on Oct 10, 2017

    @saimn
    Contributor

    Is it related to the random bug that was happening before v2.0 release ? #6291 (which looks similar), which lead to #6346 and other issues / PRs. I remember when looking at this, I was stuck with columns using a wrong format function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions