Forwarding?

The Repo on GitHub

Something about the Bot class bugs me. Python must have a good way to do this. (We find a way. Is it good?)

Bots know a lot of facts about themselves. Most of what they know, they receive from the World, and they also have a few thoughts of their own. The info from World is stored in a Bot’s saved instance of the Knowledge class, and the Bot accesses that info via lots of properties, like these:

    @property
    def id(self):
        return self._knowledge.id

    @id.setter
    def id(self, id):
        self._knowledge.id = id

    @property
    def direction(self):
        return self._knowledge.direction

    @direction.setter
    def direction(self, direction):
        self._knowledge.direction = direction
    ...

There are over 40 lines of code in Bot for these accessors, and all of them are just that simple. They make reading the Bot class more difficult, and more irritating, than it needs to be.

The Kotlin language has a very nice feature for forwarding, which allows you to specify a member variable and its type, and specify that your class’s interface of that type is handled by that member:

class Bot(k: Knowledge) : Knowledge by k { ... }

Surely Python, which is a jungle comparable only to Verdi or the Amazon, has some way to specify that certain methods are handled by forwarding to a member variable. Please stand by while I do a little searching, though I am almost hopeful that I’ll have to invent it myself.

Yes, well. Python has two dunder methods, __getattr__ and __setattr__, that are called when an attribute is accessed for read or write, respectively. Let’s roll our own delegation. We’ll do it with some tests to find out how what really happens. I think this might be pretty simple.

Temptation

I am tempted to do this in place, in Bot. Bot is well-tested and if we break any of its getters and setters we should get an error message. We’ll experiment in Bot and if we like how it’s going, we’ll push on, otherwise back away and try something simpler.

We’ll try it with id:

class Bot:
    @property
    def id(self):
        return self._knowledge.id

    @id.setter
    def id(self, id):
        self._knowledge.id = id

I wish I could comment out just the getter, since it is simpler. Let’s see if we can remove the setter and work with it. It won’t be much harder.

    # @id.setter
    # def id(self, id):
    #     self._knowledge.id = id

We get two tests failing. I try this:

    def __setattr__(self, key, value):
        if key == 'id':
            setattr(self._knowledge, key, value)

I quickly learn that __setattr__ is called for all the members that I try to define, so we need to support an else case.

    def __setattr__(self, key, value):
        if key == 'id':
            setattr(self._knowledge, key, value)
        else:
            super().__setattr__(key, value)

PyCharm actually suggested that line. Nice work, JetBrains!

Tests are green. I’m not going to commit this just yet, however. Let’s do the getter:

    def __getattr__(self, key):
        if key == 'id':
            return getattr(self._knowledge, key)
        else:
            return super().__getattribute__(key)

My tests are running. PyCharm suggested __getattribute__ instead of __getattr__. I need to understand why, and whether that’s the right thing to do.

After a quick read

If my understanding is correct, __getattribute__ is the “top-level” method for attribute access, and it will call __getattr__ if the attribute is not found. So PyCharm is correct to call __getattribute__, so that our Knowledge instance will be accessed just as it would be with a direct call. I suspect that __getattr__ might work, but I’m prepared to go with the received knowledge on this one. I’ve made a note to do further study.

Now I think we can do something a bit more sensible. We’ll have a list of keys that we will handle in our get and set, and defer the rest.

FORWARDS = ['id',]
class Bot:
    def __getattr__(self, key):
        if key in FORWARDS:
            return getattr(self._knowledge, key)
        else:
            return super().__getattribute__(key)

    def __setattr__(self, key, value):
        if key in FORWARDS:
            setattr(self._knowledge, key, value)
        else:
            super().__setattr__(key, value)

We’re green. And I am liking this. Tempted to commit but let’s do one more first.

FORWARDS = ['direction', 'id',]

Works. Green. Commit: moving properties to FORWARD and forwarding __getattr__ and __setattr__.

I remove the holding property and nothing breaks. I think what we’ll do is implement it anyway, as it is one of the members of knowledge and if we ever do go after it, we’d better be ready.

This brings up an issue though: What about these setters? Why are they there? The Bot itself should never really set anything into _knowledge, should it? So these must be for tests.

Further work with this idea turns up an issue. I believe that the knowledge-setters are only called from tests: Bot itself considers the _knowledge member to be read-only. But tests, for example, do things like this:

    def test_nothing_near(self):
        world = World(10, 10)
        client_bot = DirectConnection(world).add_bot(5, 5)
        real_bot = world.map.at_id(client_bot.id)
        client_bot.direction_change_chance = 0.0
        real_bot.direction_change_chance = 0.0

        client_bot.vision = []

        client_bot.do_something_only_for_tests(DirectConnection(world))
        world.update_client_for_test(client_bot)
        assert client_bot.location == Location(6, 5)
        vision = client_bot.vision
        assert ('R', 6, 5) in vision

Now I think I can remove this one and everything will be just fine. Yes. There is a test that sets a bot’s id. I can remove that one, and I do.

I was mistaken about Bot never setting into _knowledge: there is code in Bot that sets a bot’s id.

    def _create_new_bot(self, bot_id):
        from client.bot import Bot
        new_bot = Bot(0, 0)
        new_bot.id = bot_id
        self.bots[bot_id] = new_bot
        return new_bot

The id, if all goes well, will go into the new bot’s knowledge, and it will be overwritten by the new knowledge when it is updated in just a moment. I think we can remove that line.

OK, new rule: we will not allow setting of any of those members. Here we are, green as grass:

FORWARDS = ['direction', 'holding', 'id', 'location', 'vision']
class Bot:
    def __init__(self, x, y, direction=Direction.EAST):
        self.name = 'R'
        self.direction_change_chance = 0.2
        self._knowledge = Knowledge(Location(x, y), direction)
        self.state = Walking()
        self._old_location = None

    def __getattr__(self, key):
        if key in FORWARDS:
            return getattr(self._knowledge, key)
        else:
            return super().__getattribute__(key)

    def __setattr__(self, key, value):
        if key in FORWARDS:
            raise KeyError(f"cannot set _knowledge attribute '{key}'")
        else:
            super().__setattr__(key, value)

Let’s review Knowledge just to see what else it knows. Interestingly, whatever it is, we aren’t making much use of it.

class Knowledge:
    def update(self, update_dictionary):
        self.direction = update_dictionary['direction']
        self.id = update_dictionary['eid']
        self.location = update_dictionary['location']
        self.receive(update_dictionary['held_entity'])
        self._scent = update_dictionary['scent']
        self.vision = update_dictionary['vision']

I notice that Knowledge has one item from the client side, the _energy. I think we’ll find that that’s there because the state machine wants it.

class Knowledge:
    def has_energy(self):
        return self._energy >= self.energy_threshold


class Walking:
    def action(self, _knowledge):
        return []

    def update(self, knowledge):
        knowledge.gain_energy()
        if knowledge.has_energy():  # <===
            if knowledge.has_block:
                return Laden()
            else:
                return Looking()
        return Walking()

Right. We notice a bit of a bump here: the Bot has some personal info that’s not in Knowledge, such as its _old_location, and some personal info that is, _energy. So only part of our Knowledge is updated from the World. We might want to formalize that a bit and smooth out the bump. That’s not for today.

I think we are solid. Let’s commit: use getattr and setattr to manage forwarding to knowledge. No setting allowed. Oops, left a test broken where I had tested the setter. Commit again with oops message. And let’s do test that:

    def test_cannot_set_into_knowledge(self):
        bot = Bot(10, 10)
        with pytest.raises(KeyError):
            bot.id = 101

OK. Better sum up.

Summary

My thoughts got ahead of me this morning. I was juggling too many thoughts, what about setters and do we want two lists, one for getters and one for setters, and I’m not sure what else. The result was a bit of chaos at the end there, with a commit of a test that needed a set removed, and then somehow I typed or pasted the commit message into the middle of a line of code and committed that. We’re all fine here now, thank you.

Uh, we had a slight weapons malfunction, but uh… everything’s perfectly all right now. We’re fine. We’re all fine here now, thank you. How are you?
– Han Solo, public communication

The best question to ask about this set of changes is whether they’re too clever. We should ask that whenever we use a dunder method, I suppose, but these in particular are rather deep in the bag of tricks. They do save upwards of 40 lines of boilerplate, at the cost of 11 lines of dunder code. And I think I am pleased about changing things so that the setters do not work. That has at least two possible payoffs:

First, if we ever do need to rig up some kind of specialized knowledge objects, it’ll be good to identify ill-considered “quick fixes” like “oh, we’ll just set the id here”. The error will cause us to think, with a chance at a better way of doing things.

Second, we have learned a way that we could use to make certain attributes of an object “read-only”. One of Python’s biggest gaps, in my view, is that you can basically set any value into any object. If you have an object in hand and say obj.fargle = 31, that object now has a fargle attribute with the value 31.

This test passes:

    def test_random_attribute(self):
        bot = Bot(10, 10)
        bot.fargle = 31
        assert bot.fargle == 31

That means that, in Python, you’re always just one typo away from storing some random item somewhere, and that defect is going to be hard to find. I understand why it is that way, but I am not fond of the “feature” even so.

We might be able to make use of our improved understanding of the attr methods to make key objects read only. That bit of insight may pay off in the future, and I’m glad to have a bit better understanding of how we might do it.

I might ask my colleagues tomorrow night what they think about this change. I am sure they’ll have some objections. I plan to listen carefully and then ignore them. But we’ll see. And if you have thoughts on the subject, hit me up on the pachyderm.

For me, today, I’m happy with what we’ve done. I hope to see you next time!