Session adds non-statemanager to list of state managers

[goddard@…]

The following bug report has been submitted:
Platform:        Darwin-18.6.0-x86_64-i386-64bit
ChimeraX Version: 0.91 (2019-06-24)
Description
The following traceback was created by my adding code that raised an exception.  It is not an error.  But it reveals that Session.__setattr__() is adding ses._last_label_view which is a Place instance to the list of state managers.  "Place" is not a StateManager and also this attribute is private (leading underscore) so it should not be saved in sessions.

This is the cause of bug #2106 where Place.reset_state() is called on a Place instance.

Probably Session.__setattr__() should only recognize StateManager values that are not private.


Log:
UCSF ChimeraX version: 0.91 (2019-06-24)  
© 2016-2019 Regents of the University of California. All rights reserved.  
How to cite UCSF ChimeraX  

> open 3cl0 format mmCIF fromDatabase pdb

3cl0 title:  
N1 Neuraminidase H274Y + oseltamivir [more info...]  
  
Chain information for 3cl0 #1  
---  
Chain | Description  
A | Neuraminidase  
  
Non-standard residues in 3cl0 #1  
---  
CA — calcium ion  
G39 —
(3R,4R,5S)-4-(acetylamino)-5-amino-3-(pentan-3-yloxy)cyclohex-1-ene-1-carboxylic
acid (Oseltamivir carboxylate)  
  
3cl0 mmCIF Assemblies  
---  
1| author_and_software_defined_assembly  
  
  

> select /A:391

6 atoms, 5 bonds, 1 model selected  

Expected a keyword  

> usage label

label [objects] [objectType] [text text] [offset offset] [color color]
[background background] [size size] [height height] [font a text string]
[onTop true or false]  
— Create atom labels  
objects: an objects specifier or nothing  
objectType: one of atoms, bonds, pseudobonds, or residues  
text: default or a text string  
offset: default or some numbers  
color: default or a color  
background: none or a color  
size: default or an integer  
height: fixed or a number

label delete [objects] [objectType]  
— Delete atom labels  
objects: an objects specifier or nothing  
objectType: one of atoms, bonds, pseudobonds, or residues

label listfonts  
— List available fonts

label orient [orient]  
— Set label orientation updating  
orient: a number  

> label sel text boom

> label orient 45

Traceback (most recent call last):  
File
"/Users/goddard/ucsf/chimerax/ChimeraX.app/Contents/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-
packages/chimerax/core/triggerset.py", line 130, in invoke  
return self._func(self._name, data)  
File
"/Users/goddard/ucsf/chimerax/ChimeraX.app/Contents/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-
packages/chimerax/label/label3d.py", line 395, in _update_graphics_if_needed  
ses._last_label_view = cpos  
File
"/Users/goddard/ucsf/chimerax/ChimeraX.app/Contents/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-
packages/chimerax/core/session.py", line 453, in __setattr__  
self.add_state_manager(name, value)  
File
"/Users/goddard/ucsf/chimerax/ChimeraX.app/Contents/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-
packages/chimerax/core/session.py", line 466, in add_state_manager  
raise RuntimeError('Got last_label_view')  
RuntimeError: Got last_label_view  
  
Error processing trigger "graphics update": Got last_label_view:  
RuntimeError: Got last_label_view  
  
File
"/Users/goddard/ucsf/chimerax/ChimeraX.app/Contents/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-
packages/chimerax/core/session.py", line 466, in add_state_manager  
raise RuntimeError('Got last_label_view')  
  
See log for complete Python traceback.  
  




OpenGL version: 4.1 ATI-2.9.26
OpenGL renderer: AMD Radeon Pro 580 OpenGL Engine
OpenGL vendor: ATI Technologies Inc.

[Tom Goddard]

I think Session.setattr() should not even be detecting StateManagers for session saving. Instead code that wants to add a StateManager for session saving should have to explicitly call Session.add_state_manager(). The current code is a nightmare to debug since there is no explicit call to indicate when a state manager is added, instead it is a side-effect of setting an attribute. While this saves typing an extra line to add a state manager it makes debugging very difficult -- I spent an hour figuring out how bug #2106 could happened.

[Greg Couch]

Conrad, Eric, please chime in. Tom has requested that we go back to the previous API where state managers were explicitly added to the session. I don't care :-). Let's discuss.

[pett]

Yeah, I disagree with Tom though I do agree that at the very least setattr should be narrowed to only add objects inheriting from StateManager as state managers, and only if the attribute name does not begin with an underscore. For efficiency, we should probably discuss it when we are all at work at the same time. :-)

[Conrad Huang]

I don't really care. I always thought setting an attribute having magic side effects was ... unusual, but I don't care. Just let me know how it works.

[pett]

After discussion with Greg, the short-term plan is to fix Session.setattr to only add state managers. The short-but-not-quite-as-short-term plan is to have state managers register themselves as part of their constructors, and remove Session.setattr entirely.

[pett]

Okay, "shortest term" solution is in there.

[pett]

There are eleven classes listed in Session.snapshot_methods that are treated like state managers even though they don't inherit from StateManager. T.G. needs to find the places in his code that add such objects as session attributes and make explicit add_state_manager calls before I can remove the setattr behavior.

[pett]

I should mention that when those changes are made the test in setattr should also be changed from:

if self.snapshot_methods(value, bar_type=StateManager) is not None:

to:

if isinstance(value, StateManager):

which will allow things to work until I then change all StateManagers to register themselves in their constructor.

[Tom Goddard]

The change to ignore Session underscores broke color zone and surface zone session saving, bug #2221. I fixed that by adding the now needed Session.add_state_manager(). There could be other things no longer saved in sessions due to that underscore change.

[pett]

After a lot of headache-inducing code examination, concluded it would be too awkward to remove the StateManager registration from session.setattr, principally because the state managers are stored as a dictionary whose key is the same as the attribute name for those state managers, and that fact is central to session save/restore.

Instead, I added an init_state_manager method to StateManager that anonymous state managers can use to easily add themselves to the session (generates a unique tag to use, deregisters on destroy() and so forth).

[goddard@…]

I don’t quite grasp this.  I have various state managers that use an underscore attribute session._some_state_mgr and call session.add_state_manager().  On session restore the state manager sets the attribute set session._some_state_mgr.  So these don’t use Session.__setattr__() to add a state manager and don’t need that mechanism.  So I’m not sure why you concluded that other code does need that mechanism.  I guess it is probably because these other state managers don’t set their session attribute on restore and the __setattr__() version makes sure the attribute gets set?  Or maybe I am just missing something about the meaning of “tag” in add_state_manager(tag, state_manager).  Does tag have to be an attribute name for Session?  If so, this needs documenting in the add_state_manager() doc string.

[pett]

The idea was that state managers wouldn't _have_ to explicitly call add_state_manager, that their constructor would handle that. Unfortunately, add_state_manager requires a 'tag' argument, which needs to match the attribute name used to assign it as a session attribute if it is going to be restored as a session attribute. The constructor doesn't know the proper tag to use unless you pass it into he constructor. Which means that all classes inheriting from StateManager need to call the StateManagerinit (which none currently do, since there is no init now), pass in the tag, and either assign the manager into the session themselves or have the StateManager constructor do it "under the hood"(!). It just did not seem like an improvement.

The method I added to StateManager (init_state_manager) is a convenience method for "anonymous" state managers that have no need to assign themselves as attributes of the session. For instance, the rotamers GUI is going to issue a command to actually depict the rotamers in the scene. Those rotamers need to react to the base residue/structure being deleted, and the GUI needs to be able to react to rotamers being deleted, and these reaction capabilities (i.e. trigger handling) needs to survive session save/restore. Therefore the command is going to create an "anonymous" state manager to handle those needs (and return it as the command result) -- since there might not even be a GUI!. So, in most cases there would probably only be one of these state managers at a time, but there could be dozens. So I definitely _don't_ want to assign these state managers as session attributes.

[Conrad Huang]

What if we made the tag default to None and not assign a session attribute in that case?

Also, from our brief discussion, it sounds like you want a separate StateManager instance for each set of rotamers. Does it make more sense to have a single RotamerStateManager and separate RotamerState for each set?

Just asking.

[pett]

Tags have to be unique since they are used as keys by the session in a dictionary. The init_state_manager function I wrote handles the task of generating a unique key for state managers that don't care what their tag is.

I don't think there is any particular advantage to having an overall RotamerStateManager. It's slightly more complex to code than one-off state managers, so I decided against it. I guess it does have the advantage of only needing a single tag in the session, but that's it as far as I can see.