Repository navigation
Cache table formatting funcs in col.info not pprint module - #6714
Conversation
|
Hi there @taldcroft 👋 - 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 labelled 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 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here |
|
This does make an API change to |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
This all looks good to me. I would indeed not worry about the API change to what is a private function.
I do wonder a bit about the test, since you cannot reproduce the problem (and the test likely is fairly slow). But perhaps better than nothing.
|
@mhvk - if @larrybradley cannot get this test to fail in master then I would agree to cut it out. Or maybe just cut anyway if his demonstration jupyter notebook fails with master but works with this PR. I am getting more aware these days of test times and ensuring that they are meaningful. |
|
Thanks, @taldcroft! I cannot get the new test to fail for this PR or master. However, my notebook fails for master, but passes with this PR. Feel free to remove the test since this bug occurs randomly. |
c43ddd8 to
1871e1f
Compare
| _serialize_context = None | ||
|
|
||
| def __init__(self, bound=False): | ||
| super().__init__(bound=bound) |
There was a problem hiding this comment.
If this is backported it needs the arguments for super. Regard this as just a note for the "backporter".
There was a problem hiding this comment.
@MSeifert04 - Please use the "manual backport" label for these cases, as I'm not reading through the comments when doing backports ;)
There was a problem hiding this comment.
And so begins the predicted pain of backporting Py3-only code to the LTS... hopefully it will be manageable.
There was a problem hiding this comment.
Funnily enough, this was quite painless, and the real conflicts are in pprint.py...
(Though the heads up is really needed as this part obviously wasn't conflicting)
|
Note, since the last force push only deleted a new test (that was deemed useless), I stopped Travis after the initial tests. |
Cache table formatting funcs in col.info not pprint module
Fixes #6703
"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)."
This PR moves the cache dict (
_format_funcs) into the columnInfoobject instead of being a common dict in thepprintmodule. This should fix the issue since the cache is specific to the column. I added a test but was not able to reproduce the problem on my Mac with current master. It would be good if @larrybradley can try demonstrating that the new test fails on master but passes with this PR.The 7369e5a commit fixes a minor unrelated bug where the exception catching for a bad format actually raised an unexpected exception for a mixin column that has no
nameattribute. This is a leftover of adding mixing columns where one should always usecol.info.namenotcol.name.