My class method became a instance method

[Baldyr]

I had a weird experience over the last day or so. Since I converted my PyScenario application to work with another version of the parent mod (basically a mod-mod) one class object couldn't be pickled anymore. The exception complained over pickling an instance method and as I had no idea what that was, I researched the topic as good as I could. Even if was a little unsure about what it actually was I at least knew that I had no instance methods part in my program, but somehow the pickling application was identifying some method as part of an instance rather than a class, or something.

I believe that I managed to pinpoint the actual method and it has been unchanged during several months of beta testing, so it was the least likely suspect. It turned out to be a method I basically used for adding a class instance to a list. (That list is part of another class, of which an instance is stored in a list in yet another class instance, which in turn is stored in a list in an instance. So its basically a whole lot of instances with list attributes.)

I tried everything imaginable, basically reverting all changes in anyway connected to this particular issue, but it was still registering as a instance method. And even more strangely - only when the pickling was caused by onPreSave the first time initiated by onGameStart. The solution was to simply skip the method and assign the object directly to the list with the append() method.

Any ideas on how such a thing could occur? If anyone is interested I could post some code, but as I said - there are layers of stuff happening so it would be pretty hairy.

 

[EmperorFool]

Yeah, without code I can't even begin to guess. It may be that you are inadvertently trying to pickle the method itself as a data object which could happen if you pickle a class instead of an instance of the class.

A class is an object like everything in Python, and an object is just a special dictionary. When you pickle an object--an instance of a class--you don't pickle any of the methods. But if you pickle the class itself which contains all the methods, it will try to pickle those methods. I'm surprised that's a problem, but it's the only thing that makes sense without any further information.

Always post at least the stack trace.

 

[Baldyr]

Sorry about that.

Spoiler :

Traceback (most recent call last):

File "CvEventInterface", line 23, in onEvent

File "CvRFCEventManager", line 108, in handleEvent

File "CvRFCEventManager", line 119, in _handleDefaultEvent

File "CvRFCEventHandler", line 721, in onPreSave

File "PyScenario", line 75, in save

File "e:/main/civilization4/warlords/assets/python/system\copy_reg.py", line 69, in _reduce_ex

TypeError: can't pickle instancemethod objects
ERR: Python function onEvent failed, module CvEventInterface

This exception only occurs when the call is made the first time (onPreSave). After that it seems to work just fine.

I did some more thinking and tested a hunch - and it turns out the problem was some seamlessly redundant code that has been laying around since the inception of the program. Because when I commented it out, I can suddenly use the method that I though was causing the issue. Now I believe its another method...

class Actions(Trigger):
        
        def __init__(self, pContext=None):
                self.list = []
                self.unitList = []
                if pContext:
                        self.Context = pContext
                self.Actions = self

        def appendUnit(self, pAction, pTrigger):
                self.unitList.append(pAction)
                self.setUnitValues(pAction, pTrigger)

[B]        def setUnitValues(self, pEntry, pTrigger):
                pEntry.player = pTrigger.player
                pEntry.plotList = pTrigger.plotList [/B]

Looking at the code I'm thinking that the pTrigger parameter is also redundant with how the code is structured today.

The call to Actions.appendUnit() comes from this method belonging to the Trigger class:

        def units(self, iX, iY, eType=None, iNum=1):
                self.setCoords((iX, iY))
                self.Actions.appendUnit(Units(iX, iY, eType, iNum)[B], self[/B])
                return self

This is actually a good development because it gives an opportunity to trim my code.

 

[EmperorFool]

Why does each Actions instance have a reference to itself called Actions?

self.Actions = self

Apparently the Trigger class already defines an Actions attribute, ostensibly pointing to itself, and then Actions extends Trigger and thus inherits that attribute. Oh, no it doesn't. Why doesn't Actions.__init__() call Trigger.__init__()? Each constructor should 99999 times out of 100000 call its parent constructor.

class Actions(Trigger):
        
        def __init__(self, pContext=None):
                Trigger.__init__(self)
                self.list = []
                self.unitList = []
                if pContext:
                        self.Context = pContext
                self.Actions = self

Does each of these classes take a pContext parameter? If so, you can have the root parent class handle storing it and pass it along from each child.

class Trigger:
        
        def __init__(self, pContext=None):
                self.Context = pContext
                self.Actions = self

class Actions(Trigger):
        
        def __init__(self, pContext=None):
                Trigger.__init__(self, pContext)
                self.list = []
                self.unitList = []

As you can see, the Actions constructor only needs to do whatever the Trigger constructor doesn't. Same for any class that extends Trigger. Finally, I recommend always assigning pContext to self.Context, even if it's None. That way the attribute always exists and can be tested more easily. Another option is to assign an empty context to self.Context if pContext is None. That way you never have to check if self.Context exists or is None--just use it.

Okay, really finally, I suggest naming your attributes with an initial lowercase letter like methods. Only classes and modules should have an initial capital. It only makes it easier for us all to read each other's code. I saw self.Actions and immediately thought you had assigned the class to an attribute of an object, and this would cause the problem you are having if that object were pickled.

 

[Baldyr]

Oh, my. There really is old code lying around all over the place. I should probably clean more often?

Why does each Actions instance have a reference to itself called Actions?

Its a very good question. The only thing I can think of is that its part of some old setup not used anymore...

Why doesn't Actions.__init__() call Trigger.__init__()? Each constructor should 99999 times out of 100000 call its parent constructor.

Eh, what? I don't think I've ever done anything like that, but perhaps I should. I always give each sub-class its independent __init__ definition. I did use super at one point to do something similar, but haven't really found any use for it with the current setup.

How my classes work is that they only share methods - but not __init__. So its basically organizing things in a hierarchy so that identical methods aren't duplicated. Perhaps I could have them share methods without being sub-classes then? (How, exactly?)

Clearly I'm in the dark about something here...

Does each of these classes take a pContext parameter?

Oh, the default pContext=None thing is just something I was working on before - not in use at the moment. There are never any pContext passed along when Actions instances are crated, so that could as well be taken out. (But I might still implement the feature I was working on, and the default value seems harmless enough.)

Okay, really finally, I suggest naming your attributes with an initial lowercase letter like methods. Only classes and modules should have an initial capital. It only makes it easier for us all to read each other's code.

I made up my own naming conventions for this module a long time ago, and haven't bothered with changing them even when I became aware of the these issues. The reason would be that the code has grown so vast that I don't dare to make total overhauls because of all the testing it would involve.

But I might do that now. (Thankfully I have a dedicated beta-tester...)

I saw self.Actions and immediately thought you had assigned the class to an attribute of an object, and this would cause the problem you are having if that object were pickled.

Aha!

 

[EmperorFool]

The __init__() function--the class constructor--is there to set up the instance (self) attributes. Normally when one class subclasses another, the subclass is a more specific type of the generic base class, and thus it would receive whatever attributes the parent assigned normally and then add/override what it does differently.

Inheriting methods is a large part of subclassing, no doubt, but if the only point is to make those functions available to other classes you don't need to subclass. If the parent class doesn't assign any attributes to self, you don't gain anything by calling the parent's constructor. As with most things in Python, there is no one correct way.

 

[Baldyr]

Ok, makes sense. My classes all have their own instance fields - so they don't share any with the parent. The fields that are shared (global settings) come with a Context instance that is recreated every time.

 

[EmperorFool]

Instance fields are never shared. They are called that because each instance has their own value--even if they all have the same value. No, the reason to call the parent constructor is to have it set up the child as if it were a parent class instance.

It's tough talking in these generalities. Let's use a simple example: shapes. Now, first this isn't how I would normally design these classes, but they make a good example. The base Shape class defines an anchor point from which it is drawn. Each actual shape subclass uses that anchor point differently, but the fact that they all share the concept of an anchor point makes it a good candidate to put into the superclass.

class Shape:
    def __init__(self, anchor):
        self.anchor = anchor
    def draw(self, pen):
        pen.moveTo(self.anchor)

Very simple. Now, instead of having every subclass of Shape store the anchor point itself, they pass it to the superclass. Also you can see that the draw() method moves the "pen" to the anchor point. Assume that pen is an object with simple drawing commands on the screen such as moving, drawing lines, drawing circles, etc.

class Square:
    def __init__(self, anchor, size):
        Shape.__init__(self, anchor)
        self.size = size
    def draw(self, pen):
        Shape.draw(self)
        pen.line(self.size, 0)
        pen.line(0, self.size)
        pen.line(-self.size, 0)
        pen.line(0, -self.size)

Square inherits some behavior from Shape. It has its own anchor field which the Shape class sets onto it. Notice that Square.__init__() passes the Square instance "self" to the parent __init__() method. The "self" in any Shape method can be a Shape instance or an instance of any of its subclasses. In that sense a Square "is a" Shape--it's a more specific shape. Same for other subclasses:

class Square:
    def __init__(self, anchor, radius):
        Shape.__init__(self, anchor)
        self.radius = radius
    def draw(self, pen):
        Shape.draw(self)
        pen.circle(self.radius)

Now, Shape is but five lines--only two that do any real work--so it's hard to see what this buys you. But if you look at other sample code (e.g. BUG Mod) you'll find many examples of base classes that do a lot of the grunt work so the subclass only does what is specific to it.

The beauty is that you can pass around Shape instances (Squares, Circles, Rectangles, Polygons, whatever) and get their specific behavior when you call draw() on them without needing to know anything about them. They are all Shapes--they have an anchor point and can receive the draw() method.

 

[Baldyr]

I was just using incorrect terminology, I guess.

Now, first this isn't how I would normally design these classes, but they make a good example.

Agreed on the goodness of the example, but the question beckons: How would you normally design those classes?

 

[EmperorFool]

It would depend a lot on how they were being used. For a straight-up drawing application this isn't far off the mark. Another method would be to define the bounding box as a common property. There's rarely an absolutely best design.

 

[Baldyr]

Ok, the implementation would of course be situational. I was just wondering if you had another example on the same theme, because I find "professional" code to be... inspirational. One day my head will hopefully be wired in a way that makes such... beautiful code.

Also, I find it difficult to talk about different ways of doing things without using the word "method" all the time. Because it makes it very confusing. Like:

Another method would be to define the bounding box as a common property.

I had to read the paragraph twice to realize you weren't talking about defining a method. (Or were you?)

What would a "common property" be then, by the way?

 

[EmperorFool]

I definitely meant "a way of doing something" there, but yes it can get confusing in the beginning. By "common property" I meant a property (field/attribute) defined in the parent class (Shape) so that every subclass inherited its definition. For example, numberOfLegs and numberOfArms would be common properties of a class hierarchy starting with Animal.

There are tons of better examples already written, many of them free. Let Google be your friend. If you want some fairly good examples of OO design related to Civ4, check out BUG. Some of the Util modules in the Python/BUG folder actually employ good OO principles with inheritance.

 

[Baldyr]

Yeah, I really should take a closer look at BUG. Still.