Added hooks registered per model

[tcstewar]

This is another implementation of hooks (see #957 and #953 for the previous attempts).

In #953 , the hooks were implemented by just having magic variables on_step, on_pause, etc, and if you happened to define these, then the GUI would call them. However, this is a) rather magical, and b) rather non-discoverable, and c) rather annoying to debug if you're using a branch that doesn't support them.

So, @jgosmann implemented a decorator-based approach. It interacted with the ExecutionEnvironment system in nengo_gui to figure out which model we should be attaching the hook to (since we might have multiple pages open looking at different models). This works great. However, if we run outside of the ExecutionEnvironment (which happens when embedding inside Jupyter notebooks), then the hooks are attached globally. This is fine for multiple open notebooks (since those are run in different kernels), but if there are multiple cells in the notebook that use nengo_gui, or if you re-execute a cell that uses hooks, then you will now have the same hook running multiple times.

In this PR, I do the decorator registration approach, but you also have to specify the model (i.e. the nengo.Network instance) that you're attaching the hook to. This means we don't have to do anything with the ExecutionEnvironment, and it works fine and consistently in both normal nengo_gui and in Jupyter notebooks.

The syntax for this hook system is as follows:

model = nengo.Network()

@nengo_gui.hooks.on_start(model)
def on_start(sim):
    print('Start')
    
@nengo_gui.hooks.on_close(model)
def on_close(sim):
    print('Close')
    
@nengo_gui.hooks.on_pause(model)
def on_pause(sim):
    print('Pause')
    
@nengo_gui.hooks.on_continue(model)
def on_continue(sim):
    print('Continue')
    
@nengo_gui.hooks.on_step(model)
def on_step(sim):
    print('Step')

[tbekolay]

Hm... I'm not 100% sold on passing in the model, but I'm OK with it going in for now as long as we're not guaranteeing this for all future time.

[tcstewar]

Hm... I'm not 100% sold on passing in the model, but I'm OK with it going in for now as long as we're not guaranteeing this for all future time.

I can't think of any way to do it (and have it work as expected in jupyter notebooks) without passing in the model.

That said, I don't think we should merge this yet. If students need this functionality during the summer school, then we'll switch to this branch, but I'd prefer a consensus on this being the best approach before merging this into master (mostly for backwards-compatibility reasons)

[Seanny123]

@tbekolay would you mind elaborating as to why aren't you sold on passing the model?

[tbekolay]

would you mind elaborating as to why aren't you sold on passing the model?

I haven't thought about it too much, but the two main things that come to mind are:

1. It becomes a little unclear who "owns" the hooks. Something like @nengo_gui.hooks.on_step makes it clear that the GUI has a step function (presumably called on every timestep), and by using the decorator you're hooking into the GUI's step function.

   For @nengo_gui.hooks.on_step(model), it seems more like the model owns the hook, so it is not unreasonable to think that the model (network) itself has step function that I'm hooking into. If I don't look it up, I might misremember it as @model.hooks.on_step.

2. In a similar vein as 1., if the hook is "owned" by the model, then it seems logical that I would hook into other parts of the simulation. What happens if I do @nengo_gui.hooks.on_step(my_node)? I know nodes have a step function, so why not hook onto that? If hooks are network specific, then what about @nengo_gui.hooks.on_step(subnetwork)? Whether we allow that or add code to disallow it, the API presents a space of possibilities and requires us to work to narrow it down to what is actually possible.

[Seanny123]

I think those are legitimate concerns, however I can't imagine alternatives due to a limited imagination. Are there any alternatives?

[tbekolay]

The alternative is the original proposal, with @nengo_gui.hooks.on_step; however, with the way the GUI is architected right now it is technically infeasible to do it that way. I think it could be done in the rewritten version, so I would say for now it's fine to go with passing in the model and we can revisit later.

[jgosmann]

however, with the way the GUI is architected right now it is technically infeasible to do it that way

It's only infeasible to to it in way that makes the hooks specific to the model. But I don't see how a different architecture would allow that?

[tcstewar]

One new worry has come up with this implementation: the way I've implemented the WeakKey stuff doesn't quite work, so this version has a memory leak. :( So that needs to get sorted out before this is a candidate for merging.

[tcstewar]

It becomes a little unclear who "owns" the hooks.

I definitely agree. The picture in my head was that "the gui should use this hook when running this model", and so I was sort of thinking that it's owned by the gui, but each hook is tagged with a model. I'm not sure what other options there are if we want the hooks to be model-specific, which is the use cases that I had in mind....

[tbekolay]

I guess I agree with Jan that hooks shouldn't be model-specific. If I'm in a notebook and I say that I want something to happen when I press "play", I expect that to happen for all the GUI cells in the notebook. If I didn't want this, I would make the hook, then remove the hook later in the notebook.

Hooks inside the GUI (not in a Jupyter notebook) I would expect to be specific to the model they're defined in ... but my impression was that this is possible regardless.

One compromise would be to make passing in the model an optional thing. If you don't pass in a model, then it's global in a notebook, local to the file in the GUI (which may not be consistent from a developer perspective, but I would argue is what a user would expect). If you want it to be local to a model inside the notebook, then you can pass in a model. With this, I wouldn't have to remove the hook later in the situation above, I would make it a model-specific hook.

[tcstewar]

If I'm in a notebook and I say that I want something to happen when I press "play", I expect that to happen for all the GUI cells in the notebook.

I'm having difficulty coming up with generic things I would want to have happen based on GUI events, rather than model-specific things. Usually I'm doing things like updating plots based on data from the simulator, or sending a motor command to stop a particular motor that this particular model controls, or opening/closing an i/o connection that this model needs to run. Are there other use cases that people have in mind?

Maybe my problem here is that I called these things "hooks", which is leading people to think they are generic across whatever model you're working with. What I really want is I want to be able to say "hey, when running this model, do this extra thing based on the GUI state". Is there a better name for that than "hooks"? Because it never crossed my mind during the entire development process that someone might want this to be generic across a bunch of different models within a notebook.

[tbekolay]

I don't have use cases for these hooks, just opinions on the API; the use cases you describe seem like they would be doable with Process classes (though I recognize there needs to be some additions to processes, like a close method, for all use cases to work).

[jgosmann]

So maybe this isn't so much a feature that should be implemented in the GUI, but core Nengo? Something similar has come up several times before, documented in issue nengo/nengo#1118 (there only the on_close case is mentioned, but it could be extended). Originally I wanted this to handle cleanup for nengo_detailed_neurons, but ended up solving that use case with weak references. Then it came up again with nengo/nengo-extras#53 to properly close sockets.

Even if we implement some sort of hook system, it might be worth keeping the bigger picture in mind to avoid confusion with an eventual similar feature in core Nengo.

[tcstewar]

the use cases you describe seem like they would be doable with Process classes (though I recognize there needs to be some additions to processes, like a close method, for all use cases to work).

This I don't quite follow. How would the Process class realize that I've pressed the pause button in the Simulator (so that I can send a motor stop command to a robot when that happens)? How would the Process class have access to the Simulator object so that I can extract the weights to generate the plots of the function being learned? Those are the two use cases that motivated #953 in the first place.

[tbekolay]

Sorry, been a while since I read those use cases. I can definitely see a need for a hook for the pause button, since that's not a thing that really exists outside of the GUI. But, it seems reasonable to me that if you're in a notebook with a bunch of cells hooked up to the same robot (e.g., testing two different models to drive the same robot) you would want hitting the pause button to always stop the motors, not just stop the motors for one model. Not that I'm saying it's the best workflow, but it's a possible workflow that users might expect.

Plots using information from the simulator object are something I think should be possible through other means (e.g., using probes and a plugin architecture). So I guess I can see the use case of testing out some quick hacks that can become full fledged features in the future. Which, by that logic, I can just as easily see a quick hack that tests out some change in how the GUI works as a whole and thus isn't really tied to a model.

[Seanny123]

But, it seems reasonable to me that if you're in a notebook with a bunch of cells hooked up to the same robot (e.g., testing two different models to drive the same robot) you would want hitting the pause button to always stop the motors, not just stop the motors for one model.

I don't understand this use case. When I try to imagine this use case, I can think of two sub-cases:

• One model per motor
• Multiple models per motor, which weird switching on the fly via some other control

In either case, what's stopping them from setting up a hook that turns off all motors in each model (and communicates this between models) if that's the functionality they want? Alternatively, are you saying the hook from each individual simulator not being triggered by pausing one GUI is surprising? I would be surprised if it was surprising. Everything we do currently points to each simulator being independent.

Finally, it's quite possible I'm misunderstanding where this discussion currently is. Would it be worth making a NEP to summarize what we each currently understand?

[arvoelke]

For what it's worth @nvaidyan1 ran into a use-case that I believe could be resolved by this feature. In particular, he needs an on_start hook in order to reset the state of his callable Node whenever the simulation is reset from t=0. Mentioning this here in case anyone has any advice and so that he may track this PR's progress.

[tcstewar]

In particular, he needs an on_start hook in order to reset the state of his callable Node whenever the simulation is reset from t=0.

Interesting use case, although I think that's probably more correctly done by defining your own Process. That's the recommended way of making Node functions that need to know when reset() happens.

[tcstewar]

Also, putting this here as more of a note to myself, I've been mentally toying around with a slightly different approach that I think a) deals with the ambiguities and confusions discussed above b) doesn't have the weird state-tracking/memory-leak problem, and c) also gives a consistent syntax for doing other gui interface things. The syntax would be something like this:

model = nengo.Network()

gui = nengo_gui.Config()    # not sure about this name

@gui.hooks.on_start
def on_start(sim):
    print('Start')
    
@gui.hooks.on_close
def on_close(sim):
    print('Close')
    
@gui.hooks.on_pause
def on_pause(sim):
    print('Pause')
    
@gui.hooks.on_continue
def on_continue(sim):
    print('Continue')
    
@gui.hooks.on_step
def on_step(sim):
    print('Step')

I think the instance name gui would be required (the same way we require the nengo.Network() instance to be called model). And we could then use that object to give access to any features of the GUI that we haven't exposed via the user interface. For example:

gui.kept_time = 10.0

And we might even get extravagant and allow things like

gui.pos[ens] = 0.5, 0.2

Note that when run in jupyter notebook, this should still work, even though the GUI hasn't actually been created yet.

Anyway, as I said, I'm mostly just posting this here to remind myself to try this approach out and see if it works better/more consistently than the options we've looked at so far.

[jgosmann]

I don't remember all the details of the discussion. But some thoughts:

• Requiring a fixed instance name seems like an anti-pattern to me. This requirement is not visible from the code and there won't be any error I suppose when not followed. In case of of the model variable, the GUI will at least raise an exception if it is not defined. With the gui variable, however, there is no way to know whether it is misnamed or missing because no hooks were defined.
• Why is the model argument to the decorator functions still required? Or what problem of the proposal in the initial post does the nengo_gui.Config instance solve?

From skimming my initial hooks with decorators proposal #957, it seems the main discussion point is whether rerunning a notebook cell should register another hook. This makes me wonder if the possibility to register multiple hooks is required at all. Otherwise, each registered hook could just replace the previously registered hook to solve this problem.

Another idea would be to pass in the hook functions as argument to the InlineGUI constructor. That would solve the multiple cell execution problem and allow model specific hooks, but syntax in the Nengo GUI editor would be different.

[tcstewar]

• Requiring a fixed instance name seems like an anti-pattern to me. This requirement is not visible from the code and there won't be any error I suppose when not followed. In case of of the model variable, the GUI will at least raise an exception if it is not defined. With the gui variable, however, there is no way to know whether it is misnamed or missing because no hooks were defined.

Fair enough. I'm also happy with not requiring a specific name and just searching locals() to see if there is an instance of the right class in there somewhere (and probably throwing an exception if there's more than one).

• Why is the model argument to the decorator functions still required?

Ooo, good point. That's not needed at all with this. I'll edit the above post -- thank you for catching that!

Or what problem of the proposal in the initial post does the nengo_gui.Config instance solve?

The biggest one for me is that it eliminates the massive memory leak that was caused by weird global context system that my initial implementation used (and I couldn't find any other way of implementing the above syntax). It also clears up the ownership problem @tbekolay pointed out. And it eliminates the namespace cluttering and silent failing in the original proposal ( #953 ). At least, I think it will -- I haven't implemented it yet, so we'll see.

[jgosmann]

Fair enough. I'm also happy with not requiring a specific name and just searching locals() to see if there is an instance of the right class in there somewhere (and probably throwing an exception if there's more than one).

My first reaction to this is that it is even worse. It seems to call for weird corner cases leading to weird problems. For example, in a Jupyter notebook old instances might still be accessible through locals(). I believe previous cell evaluation results might be stored.

Ooo, good point. That's not needed at all with this. I'll edit the above post -- thank you for catching that!

So from a purely syntactic standpoint, this makes the proposal equivalent to #957 except for using methods of an instance instead of global methods.

Semantically it implies slightly different things ... and it might be good to define the desired semantics first before looking at the syntax that best expresses these semantics (and is able to implement them technically). It seems the main point where views diverge is the behavior in notebooks. In particular:

• What should happen if a cell defining hooks get re-executed? (Or another cell defining hooks.)
  • Register another hook? (Then you have two, which might be undesired, but allows to compose multiple hook functions.)
  • Replace the existing hook?
  • Note that this question also applies to the newest proposal as the gui instance might be created in a different cell.
• How should this interact with multiple InlineGUI instances?
  • Do they all share the same hooks?
  • Should it be possible to provide different hooks?
• What should happen in existing InlineGUI instances if the hook functions get redefined after creating of that instance?

The biggest one for me is that it eliminates the massive memory leak that was caused by weird global context system that my initial implementation used (and I couldn't find any other way of implementing the above syntax).

Are you talking about #953? I do not see the memory leak you're speaking of there ... My implementation in #957 has a memory leak (it was just meant as proof of concept), but that is fixable without too much effort by using weak references. I'd assume that weak references should also be applicable to the memory leak you are talking about (if it is a different one).

It also clears up the ownership problem @tbekolay pointed out.

That was which model owns the hooks? I'm not sure it does without the model argument.

[tcstewar]

Fair enough. I'm also happy with not requiring a specific name and just searching locals() to see if there is an instance of the right class in there somewhere (and probably throwing an exception if there's more than one).

My first reaction to this is that it is even worse. It seems to call for weird corner cases leading to weird problems. For example, in a Jupyter notebook old instances might still be accessible through locals(). I believe previous cell evaluation results might be stored.

I'm not sure there's an alternative here. This new idea that I'm working is based around creating an instance and adding the hooks to that instance. The reason I'm exploring this is that we couldn't get any consensus about how a system based on globals would work, so I'm trying out this idea of using an instance. If I use an instance, it has to have a name. I can either a) allow people to choose whatever name they want, or b) require people to use a particular name. Is there a third option you're thinking of? I can't think of one.... As I said above, I lean towards a), just for simplicity and the fact that it makes it very clear what should happen if someone creates multiple instances, but I could also be convinced about b).

So from a purely syntactic standpoint, this makes the proposal equivalent to #957 except for using methods of an instance instead of global methods.

Yes

• What should happen if a cell defining hooks get re-executed? (Or another cell defining hooks.)

I'd go with adding another hook, just to allow composition. And I'd say in most use cases the instance would be created at the same time as generating the hook, so that re-executing the cell would be fine (note that this would be true whether or not the instances required a particular name)

• How should this interact with multiple InlineGUI instances?

I think it'd be shared for all of them by default (i.e. if the InlineGUI searches inside locals() to find the configuration). But we could also add an argument to pass that instance in (which would be somewhat consistent with having to pass the model in). We might even make passing it in required for the notebook version, if that clarifies things, but I could also see leaving the default of searching locals().

• What should happen in existing InlineGUI instances if the hook functions get redefined after creating of that instance?

The same thing comes up in the non-notebook case, since Nodes have access to the instance, and can modify the contents. So I was planning on having that just be totally okay -- the only place the information about what the hooks are is in the instance, so if someone modifies them, then that's fine. Not sure why anyone would want to, but that's okay....

Are you talking about #953? I do not see the memory leak you're speaking of there ... My implementation in #957 has a memory leak (it was just meant as proof of concept), but that is fixable without too much effort by using weak references. I'd assume that weak references should also be applicable to the memory leak you are talking about (if it is a different one).

No, I'm talking about my implementation here in this PR, which does use weak references, but still has the memory leak. I spent a few weekends trying to fix the memory leak, and couldn't, and convinced myself that I couldn't find a way to fix the leak with our current system. But it's been too long since then for me to remember what the reasons were.

That was which model owns the hooks? I'm not sure it does without the model argument.

It clarifies that models don't own the hooks, I think.

But, all of this said, I really need to spend the time to actually implement this idea. Right now, it's the idea that I'm happiest with, but I've also said that about the other ideas before they got implemented, so I really need to implement it and see if anything weird comes up. It's helpful to get this feedback, though -- thank you!

[tbekolay]

I'm not sure there's an alternative here.

One advantage of instantiating an object versus having a global thing is that we can do whatever we want during object instantiation, meaning that we can have the object add itself to some registry when the script is executed. This is arguably the main benefit of this approach over globals, though to a certain extent they are equivalent because that registry is still going to be a global stored somewhere in the nengo_gui namespace.

We might even make passing it in required for the notebook version, if that clarifies things, but I could also see leaving the default of searching locals().

I would argue strongly for requiring people to pass it in. This would solve a lot of the weird magic issues in the notebook, leaving the main oddities in the Nengo GUI environment, which we control so will be able to (hopefully) figure out.

Nodes have access to the instance, and can modify the contents. So I was planning on having that just be totally okay -- the only place the information about what the hooks are is in the instance, so if someone modifies them, then that's fine. Not sure why anyone would want to, but that's okay....

This seems like a big can of worms that I would want to keep closed if possible.

[tcstewar]

One advantage of instantiating an object versus having a global thing is that we can do whatever we want during object instantiation, meaning that we can have the object add itself to some registry when the script is executed. This is arguably the main benefit of this approach over globals, though to a certain extent they are equivalent because that registry is still going to be a global stored somewhere in the nengo_gui namespace.

I think this method actually means that we don't require a registry anywhere, which is part of why it appeals to me on an implementation level.

I would argue strongly for requiring people to pass it in.

Seems reasonable to me... I'll give that a try.

This seems like a big can of worms that I would want to keep closed if possible.

Fair enough. :)

[tbekolay]

I think this method actually means that we don't require a registry anywhere, which is part of why it appeals to me on an implementation level.

I can't see how that's going to work personally, but it seems like it's time to try it and see.

[jgosmann]

No, I'm talking about my implementation here in this PR, which does use weak references, but still has the memory leak. I spent a few weekends trying to fix the memory leak, and couldn't, and convinced myself that I couldn't find a way to fix the leak with our current system.

Ah, I completely missed that this “issue” is also an actual PR. Unfortunately, I don't see the memory leak.

Given the current state of the discussion, I would favour the following:

• Hooks are passed in to InlineGUI. Not sure whether to have multiple arguments (on_start, on_pause, etc.) or whether to pass in an object that gathers the hook in one argument. In any case it makes things explicit in the notebook, no hidden magic is going on there, and it is probably the most flexible solution (you can reuse hooks between instance or use different hooks etc.)
• In the standalone GUI, some form of decorator function is used. I'm not convinced that it is necessary to have that gui instance. The implementation from Implement hooks with register functions/decorators #957 could probably simplified.

There is, however, one more question that occurred to me: How should the decorator function behave, if it registers a hook outside of the GUI execution environment (could be in a notebook or could be in a plain script)? It could be silently ignored to make it easier to reuse code outside of the GUI, but it could also raise an exception to prevent required hooks from not being registered when used outside of the GUI.