The Repo on GitHub

Explaining the situation to you helps. Then I pick a good first step. The rest is history: Long article, small steps, excellent outcome. Good job, rubber duckie!

Let me see if I can explain, for my own benefit if not yours, what the issue is.

Rubber Duckie
Explaining to someone, even someone who isn’t here, or to one’s favorite rubber duckie, often helps set thoughts in order. This is one of those times.

Because return information from a Bot action isn’t really immediate, we can’t do the obvious thing, which would be something like this:

# state looking
if we have a block in front of us:
    attempt to take the block
    if we got the block:
        state = laden
    else:
        state = looking  # still

Instead we wind up with code like this:

    def looking(self):
        if self.bot.inventory:
            self.tired = 5
            self._state = self.walking
            return
        if self.bot.can_take():
            self.bot.take()

The problem here, well one of the problems here is that at step N, while looking, we see a block and try to take it, and then return from the machine. In step N+1, still in looking, we will note that we have a block and if so, change state. But doing so means that we also skip a turn at doing anything. Right now, that’s somewhat OK because we always try to take a step and that’s all that we would have done anyway. But in principle, we missed a chance for action.

What I believe I want to do is to have the machine have two operational methods, perhaps update and act, where on update we pass in all the info the bot needs and may set the state to a new state, and then act takes place, in the same turn, in the proper state.

As I sit here, I do not see steps from here to there. Machine looks like this:

class Machine:
    def __init__(self, bot):
        self.bot = bot
        self.vision = bot.vision
        self.tired = 10
        self._state = self.walking

    def state(self):
        self.tired -= 1
        self._state()
        return self

    def walking(self):
        if self.tired <= 0:
            self._state = self.laden if self.bot.inventory else self.looking

    def looking(self):
        if self.bot.inventory:
            self.tired = 5
            self._state = self.walking
            return
        if self.bot.can_take():
            self.bot.take()

    def laden(self):
        if self.bot.has_no_block():
            self.tired = 5
            self._state = self.walking
            return
        if self.tired <= 0:
            if self.bot.can_drop():
                block = self.bot.inventory[0]
                self.bot.drop(block)

Ah. One thing I know for sure is that we need to pass the bot in on the state command, because if we do not, things like the vision, which we are caching, will not be correct. (We could instead reference thru bot, but we are trying to move to a place where we send in a package of info, not the whole bot.)

Note:
It’s worth noting that part of my problem here is that I have several things going on at once, changing how it works, changing how it’s called, changing what it knows, and even changing the state transitions. It’s no wonder that I’m a bit confused.

One possibility would be to reset way back into the past. I don’t think that’s ideal, because to the extent that my mind knows anything about this, it mostly knows what’s current. So I choose, for now, to move forward.

In the course of explaining this to us, I have come up with a tentative starting plan. First, I’ll pass in the bot on the state command, and update the vision. Then, I think what I’ll do is break out the state method to call update and act, and move things around until that works. That’s going to require breaking each of the state methods, walking, looking, and laden into two methods, one for update and one for act.

In a Slack conversation last night, GeePaw was describing an approach he would take (or would have taken) to this, and it may appear later. We’ll see and I’ll point it out if it does.

OK first get the bot passed in. We are clean and green.

    def state(self, bot):
        self.bot = bot
        self.vision = bot.vision
        self.tired -= 1
        self._state()
        return self

This breaks some tests, which, I suspect, are not expecting to have to provide a vision. In Bot, vision is initialized to None. Let’s see if initing it to an empty list suffices to make the tests run: I think it should. It does not. Undo that even if it’s a good idea, we don’t need another half idea up in this thing.

OK, well past time to look at the tests and see why they are not happy. Ah, good news, it was trivial. When I did the refactoring operation, I mistakenly had the new bot parameter default to self, which meant that all my tests passed themselves as a bot. Changing self to bot about ten times fixed all the tests. We are green.

Commit: Machine state now takes bot parameter to ensure up to date.

Now the plan is to cause the state method above to execute two methods, one to update and one to act.

Let’s rename _state to _action. Green. Commit: rename _state to _action.

Now let’s add another member, _update, which is to be a pointer to the appropriate update method for the state:

class Machine:
    def __init__(self, bot):
        self.bot = bot
        self.vision = bot.vision
        self.tired = 10
        self._update = self.update_walking
        self._action = self.walking

    def state(self, bot):
        self.bot = bot
        self.vision = bot.vision
        self.tired -= 1
        self._update()
        self._action()
        return self

    def update_walking(self):
        pass

So, let me express what I’m up to. I propose that there are two operations for each state, update and action, and that update’s job is to set the two pluggable members, _update and _action to the correct pair of operations, and then action does the logic of the operation, figuring out the commands to be done.

We are green. Let’s stick with the habit of committing on each green, unless, of course, we become concerned that the most recent move was a terrible one. I think we’re OK.

Commit: update always called, currently only does update_walking, pass

I can see two ways to go. One is to put in the other update methods, and change all the state transitions to set both variables. The other would be to elaborate this first one. I think we’ll be better off to get everything in place. Let’s drive the change from these methods:

    def update_walking(self):
        pass

    def walking(self):
        if self.tired <= 0:
            self._action = self.laden if self.bot.inventory else self.looking

wherever we set _action, we need to set the corresponding _update. This code, in walking, should really be in walking_update, but let’s not make two changes at once:

This move is going to break a few things briefly I am OK with it.

First expand the last line there into an actual if/else. PyCharm does that for me:

    def walking(self):
        if self.tired <= 0:
            if self.bot.inventory:
                self._action = self.laden
            else:
                self._action = self.looking

This is still green. Commit: open conditional expression.

Now, wishful thinking:

    def walking(self):
        if self.tired <= 0:
            if self.bot.inventory:
                self._update = self.update_laden
                self._action = self.laden
            else:
                self._update = self.update_looking
                self._action = self.looking

The sky falls for want of those methods. Add them, trivially:

    def update_walking(self):
        pass

    def update_looking(self):
        pass

    def update_laden(self):
        pass

Green. Commit: all three states support update method.

Now we need to ensure that all the state changes set both members. These two do not:

    def looking(self):
        if self.bot.inventory:
            self.tired = 5
            self._action = self.walking
            return
        if self.bot.can_take():
            self.bot.take()

    def laden(self):
        if self.bot.has_no_block():
            self.tired = 5
            self._action = self.walking
            return
        if self.tired <= 0:
            if self.bot.can_drop():
                block = self.bot.inventory[0]
                self.bot.drop(block)

Readily fixed:

    def looking(self):
        if self.bot.inventory:
            self.tired = 5
            self._update = self.update_walking
            self._action = self.walking
            return
        if self.bot.can_take():
            self.bot.take()

    def laden(self):
        if self.bot.has_no_block():
            self.tired = 5
            self._update = self.update_walking
            self._action = self.walking
            return
        if self.tired <= 0:
            if self.bot.can_drop():
                block = self.bot.inventory[0]
                self.bot.drop(block)

Green. Commit: all states update both update and action member.

Now the thing is to move the update part to the update method, leaving the action part in the other method. I think we’ll want some renaming as well … but this whole thing is only an interim step on the way to class per state.

I am concerned that the upcoming changes may break tests, because when we change state on update, our action will occur in the same turn as the state change, so some of the tests may need to reflect that we don’t skip a turn any more. We’ll find out.

Here’s the first change, from this:

    def update_walking(self):
        pass

    def walking(self):
        if self.tired <= 0:
            if self.bot.inventory:
                self._update = self.update_laden
                self._action = self.laden
            else:
                self._update = self.update_looking
                self._action = self.looking

To this:

    def update_walking(self):
        if self.tired <= 0:
            if self.bot.inventory:
                self._update = self.update_laden
                self._action = self.laden
            else:
                self._update = self.update_looking
                self._action = self.looking

    def walking(self):
        pass

In the fullness of time, the walking method will return a “step” command, I think, but for now, stepping is outside the machine. Two tests break.

    def test_bot_notices_a_block(self):
        world = World(10, 10)
        bot = Bot(5, 5)
        bot.state.tired = 0
        bot.direction_change_chance = 0
        world.add(bot)
        block = Block(7, 5)
        world.add(block)
        bot.do_something()
        assert bot.state._action == bot.state.looking
        bot.do_something()
        assert bot.has(block)

The first do_something crashes:

    def can_take(self):
>       return self.vision.match_forward_and_one_side('B', '_')
E       AttributeError: 'NoneType' object has no attribute 'match_forward_and_one_side'

Our bot has no vision. Huh. I don’t see how it used to work. Let’s back out that last change. Rollback.

The test runs again. Let’s see if we can figure out how it manages to work.

In this form, our test starts in walking … we could assert that for clarity. The update does nothing and then the action changes state to looking. On the second call to do_something we’ll be in looking state:

    def looking(self):
        if self.bot.inventory:
            self.tired = 5
            self._update = self.update_walking
            self._action = self.walking
            return
        if self.bot.can_take():
            self.bot.take()

We’ll call bot.can_take() and not crash. Ah. We will have taken a step. That will cause the world to update our vision. In the new form, we have not taken a step, and will therefore not have a vision.

Let’s put the code back as we want it and fix the test to ensure we have a vision.

Change this to provide a method:

class World:
    def step(self, bot):
        location = self.bots_next_location(bot)
        self.map.attempt_move(bot.id, location)
        bot.vision = self.map.create_vision(bot.location)

Extract method:

    def step(self, bot):
        location = self.bots_next_location(bot)
        self.map.attempt_move(bot.id, location)
        self.set_bot_vision(bot)

    def set_bot_vision(self, bot):
        bot.vision = self.map.create_vision(bot.location)

And in the test:

    def test_bot_notices_a_block(self):
        world = World(10, 10)
        bot = Bot(5, 5)
        bot.state.tired = 0
        bot.direction_change_chance = 0
        world.add(bot)
        block = Block(7, 5)
        world.add(block)
        world.set_bot_vision(bot)
        bot.do_something()
        assert bot.state._action == bot.state.looking
        bot.do_something()
        assert bot.has(block)

The other test just needed a blank vision. I think that should be the default, did that before and rolled it back.

Change this:

class Bot:
    def __init__(self, x, y, direction=Direction.EAST):
        self.world = None
        self.id = None
        self.name = 'R'
        self.location = Location(x, y)
        self.direction = direction
        self.direction_change_chance = 0.2
        self.inventory = []
        self._vision = None
        self.tired = 10
        self.state = Machine(self)

To this:

class Bot:
    def __init__(self, x, y, direction=Direction.EAST):
        self.world = None
        self.id = None
        self.name = 'R'
        self.location = Location(x, y)
        self.direction = direction
        self.direction_change_chance = 0.2
        self.inventory = []
        self.vision = []
        self.tired = 10
        self.state = Machine(self)

Green. Commit: walking properly implements update and action. Renames needed?

Now let’s change the looking state to use update similarly. From this:

    def update_looking(self):
        pass

    def looking(self):
        if self.bot.inventory:
            self.tired = 5
            self._update = self.update_walking
            self._action = self.walking
            return
        if self.bot.can_take():
            self.bot.take()

To this:

    def update_looking(self):
        if self.bot.inventory:
            self.tired = 5
            self._update = self.update_walking
            self._action = self.walking

    def looking(self):
        if self.bot.can_take():
            self.bot.take()

One test breaks, I am hoping with a timing issue because we do the take sooner.

    def test_looking_goes_to_walking_then_laden_if_block_in_inventory(self):
        bot = Bot(5, 5)
        vision_list = [('R', 5, 5)]
        bot.vision = vision_list
        machine = Machine(bot)
        machine._action = machine.looking
        machine.state(bot)
        assert machine._action == machine.looking
        bot.receive(Block(2, 2))
        machine.state(bot)
        assert machine._action == machine.walking
        machine.tired = 0
        machine.state(bot)
        assert machine._action == machine.laden

        bot.receive(Block(2, 2))
        machine.state(bot)
>       assert machine._action == machine.walking
E       assert looking == walking
E        +  where looking = <machine.Machine object at 0x106960140>._action
E        +  and   walking = <machine.Machine object at 0x106960140>.walking

Ah. this test, and all of them, really needs to set both states.

    def test_looking_goes_to_walking_then_laden_if_block_in_inventory(self):
        bot = Bot(5, 5)
        vision_list = [('R', 5, 5)]
        bot.vision = vision_list
        machine = Machine(bot)
        machine._update = machine.update_looking  # <===
        machine._action = machine.looking
        machine.state(bot)
        assert machine._action == machine.looking
        bot.receive(Block(2, 2))
        machine.state(bot)
        assert machine._action == machine.walking
        machine.tired = 0
        machine.state(bot)
        assert machine._action == machine.laden

Green. Commit: looking properly implements update and action.

Now we’ll change this:

    def update_laden(self):
        pass

    def laden(self):
        if self.bot.has_no_block():
            self.tired = 5
            self._update = self.update_walking
            self._action = self.walking
            return
        if self.tired <= 0:
            if self.bot.can_drop():
                block = self.bot.inventory[0]
                self.bot.drop(block)

To this:

    def update_laden(self):
        if self.bot.has_no_block():
            self.tired = 5
            self._update = self.update_walking
            self._action = self.walking

    def laden(self):
        if self.tired <= 0:
            if self.bot.can_drop():
                block = self.bot.inventory[0]
                self.bot.drop(block)

Two tests fail. I hope they have the same issue as the last one.

First time lucky:

    def test_laden_goes_to_walking_if_no_block_in_inventory(self):
        bot = Bot(5, 5)
        machine = Machine(bot)
        machine._update = machine.update_laden  # <===
        machine._action = machine.laden
        machine.state(bot)
        assert machine._action == machine.walking

And, crossing fingers …

    def test_laden_goes_walkabout_after_drop(self):
        bot = Bot(5, 5)
        machine = Machine(bot)
        entity = Block(3, 3)
        bot.receive(entity)
        vision_list = [('R', 5, 5)]
        bot.vision = vision_list
        bot.direction = Direction.EAST
        machine._update = machine.update_laden  # <===
        machine._action = machine.laden
        machine.state(bot)
        assert bot.has_block()
        assert machine._action == machine.laden
        bot.remove(entity)
        machine.state(bot)
        assert not bot.has_block()
        assert machine._action == machine.walking

Yes! Green. Commit: all three states use update and action properly.

Let’s have a quick look at what we have wrought and do a bit of cleanup.

class Machine:
    def __init__(self, bot):
        self.bot = bot
        self.vision = bot.vision
        self.tired = 10
        self._update = self.update_walking
        self._action = self.walking

    def state(self, bot):
        self.bot = bot
        self.vision = bot.vision
        self.tired -= 1
        self._update()
        self._action()
        return self

    def update_walking(self):
        if self.tired <= 0:
            if self.bot.inventory:
                self._update = self.update_laden
                self._action = self.laden
            else:
                self._update = self.update_looking
                self._action = self.looking

    def walking(self):
        pass

    def update_looking(self):
        if self.bot.inventory:
            self.tired = 5
            self._update = self.update_walking
            self._action = self.walking

    def looking(self):
        if self.bot.can_take():
            self.bot.take()

    def update_laden(self):
        if self.bot.has_no_block():
            self.tired = 5
            self._update = self.update_walking
            self._action = self.walking

    def laden(self):
        if self.tired <= 0:
            if self.bot.can_drop():
                block = self.bot.inventory[0]
                self.bot.drop(block)

I think we’d like the names to be more similar. Let’s rename walking, looking and laden to end with action, and the three update ones to end with update instead of begin with it.

class Machine:
    def __init__(self, bot):
        self.bot = bot
        self.vision = bot.vision
        self.tired = 10
        self._update = self.walking_update
        self._action = self.walking_action

    def state(self, bot):
        self.bot = bot
        self.vision = bot.vision
        self.tired -= 1
        self._update()
        self._action()
        return self

    def walking_update(self):
        if self.tired <= 0:
            if self.bot.inventory:
                self._update = self.laden_update
                self._action = self.laden_action
            else:
                self._update = self.looking_update
                self._action = self.looking_action

    def walking_action(self):
        pass

    def looking_update(self):
        if self.bot.inventory:
            self.tired = 5
            self._update = self.walking_update
            self._action = self.walking_action

    def looking_action(self):
        if self.bot.can_take():
            self.bot.take()

    def laden_update(self):
        if self.bot.has_no_block():
            self.tired = 5
            self._update = self.walking_update
            self._action = self.walking_action

    def laden_action(self):
        if self.tired <= 0:
            if self.bot.can_drop():
                block = self.bot.inventory[0]
                self.bot.drop(block)

Commit: renames.

Now I think we should combine the two-line setting pattern into something less error-prone.

I think we’d like to define the two state members values as explicit pairs.

    def walking_states(self):
        return self.walking_update, self.walking_action

    def looking_states(self):
        return self.looking_update, self.looking_action

    def laden_states(self):
        return self.laden_update, self.laden_action

After I typed in the first one of those, PyCharm knew what I wanted and as soon as I typed the beginning of the other two, it knew exactly what to do. And that’s with the “AI” turned off!

Now I want a setter, and to use it.

    def set_state(self, states):
        self._update, self._action = states

And now use it:

Hmm … this isn’t working. Python doesn’t do what I expected. Roll back.

class Machine:
    def __init__(self, bot):
        self.bot = bot
        self.vision = bot.vision
        self.tired = 10
        self._update = self.walking_update
        self._action = self.walking_action

    def state(self, bot):
        self.bot = bot
        self.vision = bot.vision
        self.tired -= 1
        self._update()
        self._action()
        return self

    def walking_states(self):
        return self.walking_update, self.walking_action

    def looking_states(self):
        return self.looking_update, self.looking_action

    def laden_states(self):
        return self.laden_update, self.laden_action

    def set_states(self, states):
        self._update, self._action = states

    def walking_update(self):
        if self.tired <= 0:
            if self.bot.inventory:
                self.set_states(self.laden_states())
            else:
                self.set_states(self.looking_states())

    def walking_action(self):
        pass

    def looking_update(self):
        if self.bot.inventory:
            self.tired = 5
            self.set_states(self.walking_states())

    def looking_action(self):
        if self.bot.can_take():
            self.bot.take()

    def laden_update(self):
        if self.bot.has_no_block():
            self.tired = 5
            self.set_states(self.walking_states())

    def laden_action(self):
        if self.tired <= 0:
            if self.bot.can_drop():
                block = self.bot.inventory[0]
                self.bot.drop(block)

OK, I like that, we set the states at the same time. That’s as it should be. There are some tests that need the same treatment. I’ll search for accesses to the private members and fix them up. First commit: set states with single call.

Tests fixed. Commit: everyone using set_states.

Reflection / Summary

The point of all this, such as it is, is to get to a class-per-state implementation of the state machine for these ridiculously simple bots. Why? Good question, and the reason is that I want to, and my excuse is that in more complex bots we’ll surely want state machines, and this will serve as an example of how to do it.

Given how uncertain I was coming in, this morning has gone quite well. The initial decision, to break out the internal state method into update and action was a good one, and it was quite easy to put in a basic empty framework, where all the updates were doing nothing. That got the structure in place and allowed moving update logic to the update methods one at a time.

I mentioned GeePaw’s comment. He was talking about using a pair of objects for converting the state machine. Not quite what we did here, but the notion of a “pair” certainly paid off, so he gets some of the credit. I’ll keep it right here for him in case he ever asks for it.

A few tests broke, but the changes were all simple, having to do with initialization and making sure we set both action and update in the tests.

It might have been better to go to a joint setting of both update and action earlier on. That might have discovered the tests that needed updating. To make that happen, since the tests were invading the Machine’s private members, we might have had to rename the private members or searched for accesses. Still, that might have been a bit more forward-looking, but we were not to know that it was a good idea.

We went in small steps and while I did roll back a time or two, it wasn’t a retreat in confusion, just a recognition that I’d made a poor start and would do well to back up and go again. I think the last rollback was probably due to typing () when I shouldn’t have, or not typing it when I should have. I don’t know: I started over and it worked fine. That often happens, which is another good reason for frequent commits: it makes a quick rollback pretty painless.

We’re about 2 1/2 hours in, and we have a dozen commits, so that’s about one every 15 minutes, not bad at all.

I feel that this process has taken longer than it “should” have. I think I expected to get to class per state in one or two sessions and it’s going to be five or six. Fortunately, I didn’t promise anyone how long it would take, because while I am mot smart enough to do it in two days, I am smart enough to know that my guesses on how long things will take are rarely accurate.

We’re in a much better place than we were at the beginning of this session. I apologize for the length of this article but it takes as long as it takes.