Deal with __getstate__ methods by aaronayres35 · Pull Request #633 · enthought/chaco

aaronayres35 · 2021-04-07T19:44:42Z

This PR rermoves uses of the __getstate__ method when it is not needed, and cleans up its use where it is.

The __setstate__ methods can be dealt with in a follow up PR

…ut _pickle code

…ait on the class)

aaronayres35 · 2021-04-07T19:45:12Z

    # ------------------------------------------------------------------------
-    def __getstate__(self):
-        state = super(AbstractMapper, self).__getstate__()
-        for key in ["_cache_valid"]:


_cache_valid is not a trait on this class or any of its base classes

sigh. but it is a trait on the subclasses e.g. Base1DMapper so those need to be updated

Ah you are right. TBH I believe that all related traits should be transient even if they weren't perviously involved in a __getstate__method. For example other classes have _screen_cache_valid or _selection_cache_valid traits. I think I will update all of these (to be specific I am going to update any privately named cache trait) to be transient as I can't see why they shouldn't all be so

It also doesn't make sense that for some objects the _cache_valid and similar traits would be transient traits but on others not

aaronayres35 · 2021-04-07T19:48:16Z


    # The pointer to use when drawing a zoom box.
-    pointer = "magnifier"
+    pointer = Str("magnifier", transient=True)


This may be bad if pointer is meant to be a class attribute, similar to stack_index in plot_container.py.

I need to climb up the class hierarchy to see if it was previously defined as a trait. I remember Kit mentioning this being a point of confusion. I am not sure what the ideal way to avoid this confusion would be (aside from maybe a comment above saying if this is a class attribute or just setting a trait to a value. I guess if setting a trait to some value, you could do as I have done here? and explicitly do something like Str(___)

As a quick sanity check, on master I defined a class attribute thing = "A" on SimpleZoom. I then ran:

from chaco.tools.simple_zoom import SimpleZoom SimpleZoom.thing # I see 'A' printed SimpleZoom.pointer # I get an error "type object 'SimpleZoom' has no attribute 'pointer'"

So, I believe the change made here is the right one

aaronayres35 · 2021-04-07T19:49:27Z

            "_max_index",
        ]:
-            self.assertIn(key, state)
+            self.assertNotIn(key, state)


If not persisting they should not be in state, but if we are they should be. I believe these tests are doing what they are supposed to now

aaronayres35 · 2021-04-09T15:14:33Z

-            if key in state:
-                del state[key]
+        if "stack_index" in state:
+            del state["stack_index"]


stack_index is a class attribute not a trait, so I believe this is still needed

rahulporuri

LGTM with a couple of comments

rahulporuri · 2021-04-13T10:32:28Z

    # ------------------------------------------------------------------------
-    def __getstate__(self):
-        state = super(AbstractMapper, self).__getstate__()
-        for key in ["_cache_valid"]:


sigh. but it is a trait on the subclasses e.g. Base1DMapper so those need to be updated

rahulporuri · 2021-04-13T10:38:48Z


-    def __getstate__(self):
-        state = super(LinePlot, self).__getstate__()
-        for key in ["traits_view"]:


Is traits_view a transient trait? I dont see anything in the code or in the documentation saying that it is.

I was going off wha Corran said in this issue description: "This is removing the traits_view trait from the state - under normal Traits, this will never be in the state because of special handling of Views, so this whole method can be removed."
This should certainly be documented though

rahulporuri · 2021-04-13T10:45:56Z

-            "reset_zoom_key",
-            "prev_zoom_key",
-            "next_zoom_key",


im not sure where these traits were being magically defined

me either, this is the only place I was seeing them in the codebase. Perhaps they once existed but had been removed

aaronayres35 · 2021-04-13T15:34:18Z

Looks like the CI failure on windows/pyqt from the test introduced by #636 is reproducible on every PR except the original (no idea how it didn't fail on 636).

======================================================================
ERROR: test_dont_crash_on_click (chaco.tests.test_plot.TestEmptyPlot)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\chaco\chaco\.edm\envs\chaco-test-3.6-pyqt\lib\site-packages\chaco\tests\test_plot.py", line 134, in test_dont_crash_on_click
    [(1, 1), (25, 25), (50, 50), (100, 100)],
  File "D:\a\chaco\chaco\.edm\envs\chaco-test-3.6-pyqt\lib\contextlib.py", line 88, in __exit__
    next(self.gen)
  File "D:\a\chaco\chaco\.edm\envs\chaco-test-3.6-pyqt\lib\site-packages\traitsui\testing\tester\ui_tester.py", line 106, in create_ui
    process_cascade_events()
  File "D:\a\chaco\chaco\.edm\envs\chaco-test-3.6-pyqt\lib\contextlib.py", line 88, in __exit__
    next(self.gen)
  File "D:\a\chaco\chaco\.edm\envs\chaco-test-3.6-pyqt\lib\site-packages\traitsui\testing\_exception_handling.py", line 74, in reraise_exceptions
    raise RuntimeError(msg)
RuntimeError: Uncaught exceptions found.
=== Exception (type: <class 'AttributeError'>, value: '_QtWindow' object has no attribute 'handler') ===
Traceback (most recent call last):
  File "D:\a\chaco\chaco\.edm\envs\chaco-test-3.6-pyqt\lib\site-packages\enable\qt4\base_window.py", line 258, in leaveEvent
    self.handler.leaveEvent(event)
AttributeError: '_QtWindow' object has no attribute 'handler'

I also find the error quite confusing, as looking in enable, the _QtWindow class clearly has self.handler = _QtWindowHandler(self, enable_window) in its __init__ method... Also this seems like a very strange thing to be windows specific... 🤔
I will look into this more

rahulporuri · 2021-04-15T13:51:47Z


    # Is the cached color data valid?
-    _colors_cache_valid = Bool(False)
+    _colors_cache_valid = Bool(False, transient=True)


If the cache valid flags arent transient, then the caches themselves should also be transient - _levels and _colors.

I would think HasTraits would ignore traits that are private i.e. that start an _. If that isn't the case, there are a lot more private traits that we will want to mark as transient.

As you suggested, I printed state inside the ArrayDataSource.__getstate__ method on master and ran the test suite. I do see _ prefixed traits, eg _data, '_min_index', etc. So it looks like these need to be addressed

If that isn't the case, there are a lot more private traits that we will want to mark as transient.

Does this mean all private traits be transient?

As you suggested, I printed state inside the ArrayDataSource.getstate method on master and ran the test suite. I do see _ prefixed traits, eg _data, '_min_index', etc. So it looks like these need to be addressed

Sigh. I was afraid of that.

Does this mean all private traits be transient?

Let's not do that. Definitely not in this PR.

So, I imagine the code will recreate the cache if a *cache* is False, which it is by default because we're marking it as a transient. So, I think we can ignore all of the traits that are recalculated based on the *cache* flag. - i.e. nothing actionable in this PR.

rahulporuri · 2021-04-15T14:05:54Z

    # Cached list of line widths
    _widths = List


looks like this is technically a cache but like I said earlier, this will be ignored/recomputed because the _widths_cache_valid trait is False - no action required, just pointing it out.

rahulporuri · 2021-04-15T14:31:37Z

    # Is the tool always "on"? If True, left-clicking always initiates
    # a zoom operation; if False, the user must press a key to enter zoom mode.


looks like you lost the #: because of the merge conflicts.

rahulporuri

LGTM

aaronayres35 added 13 commits April 7, 2021 13:39

remove __getstate__ in abstract_data_source.py

078f382

remove __getstate__ from axis.py

3745495

remove __getstate__ from grid.py

b7f7210

remove __getstate__ from base_xy_plot.py

72700bb

remove __getstate__ from cross_plot_frame.py, also delete commented o…

77c0e59

…ut _pickle code

remove __getstate__ from lineplot.py

80a16b9

remove __getstate__ from multi_line_plot.py

fae52fa

remove __getstate__ from tools/simple_zoom.py

c7c93ab

remove __getstate__ from simple_plot_frame.py

f38cbcc

remove __getstate__ from abstract_mapper.py (_cache_valid is not a tr…

eeea965

…ait on the class)

fix array_data_source.py __getstate__ and its tests

6fc674f

update plot_containers.py

ba66fc8

delete commented out test skip

2152d9b

aaronayres35 commented Apr 7, 2021

View reviewed changes

delete _pickles commented out code

a984b40

aaronayres35 changed the title ~~[WIP] Deal with __getstate__ and __setstate__ methods~~ Deal with __getstate__ and __setstate__ methods Apr 9, 2021

aaronayres35 commented Apr 9, 2021

View reviewed changes

aaronayres35 requested a review from rahulporuri April 9, 2021 15:14

rahulporuri approved these changes Apr 13, 2021

View reviewed changes

aaronayres35 added 2 commits April 13, 2021 10:19

make all private cache related traits transient (might be overrkill)

6bfb912

Merge branch 'master' into audit-dunder-state-methods

c005b55

aaronayres35 mentioned this pull request Apr 13, 2021

fix windows/pyqt test_dont_crash_on_click failure #663

Merged

Merge branch 'master' into audit-dunder-state-methods

fd4b2c9

rahulporuri reviewed Apr 15, 2021

View reviewed changes

fix merge conflict issues

55893e0

rahulporuri reviewed Apr 15, 2021

View reviewed changes

bring back #:

f3395f0

rahulporuri approved these changes Apr 15, 2021

View reviewed changes

aaronayres35 mentioned this pull request Apr 15, 2021

skip test_dont_crash_on_click on windows/pyqt #666

Merged

Merge branch 'master' into audit-dunder-state-methods

14789fd

aaronayres35 merged commit df05047 into master Apr 15, 2021

aaronayres35 deleted the audit-dunder-state-methods branch April 15, 2021 16:18

aaronayres35 changed the title ~~Deal with __getstate__ and __setstate__ methods~~ Deal with __getstate__ methods Apr 15, 2021

aaronayres35 mentioned this pull request May 4, 2021

ArrayDataSource __getstate__ doesn't call super #248

Closed

		# Is the tool always "on"? If True, left-clicking always initiates
		# a zoom operation; if False, the user must press a key to enter zoom mode.

Conversation

aaronayres35 commented Apr 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rahulporuri Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronayres35 Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rahulporuri left a comment

Choose a reason for hiding this comment

Uh oh!

rahulporuri Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronayres35 commented Apr 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rahulporuri left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aaronayres35 commented Apr 7, 2021 •

edited

Loading

rahulporuri Apr 13, 2021 •

edited

Loading

aaronayres35 Apr 13, 2021 •

edited

Loading

rahulporuri Apr 13, 2021 •

edited

Loading