Add close method to Simulator.

[jgosmann]

Adresses #739, #857.

[jgosmann]

This does not add any global tracking of open simulators or automatic closing atexit. But it seems that we might discuss that functionality further and it can be implemented in a separate PR.

[mundya]

[Mega nit-pick!] Any reason why ValueError? I tend to read ValueError as "the thing you want to do would work with a different parameter value" rather than "from the state you're in, this thing will never work".

Also, briefly glancing at nengo_spinnakers error throwing for this, I think the error could be more explanatory, e.g, "Simulator has been closed and can't be used to run further simulations."

[tbekolay]

Yeah, I think there should be a specific exception subclass for this, similar to StopIterationError; maybe SimulatorClosedError or something. That could be done in #781 though.

[hunse]

For now, I would go with RuntimeError.

[hunse]

Specifically RuntimeError("Simulator cannot run since it has already been closed.") or RuntimeError("Simulator has been closed, so it cannot run anymore.")

[jgosmann]

Closed file object raise a ValueError when you try to read them.

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-26-bacd0e0f09a3> in <module>()
----> 1 f.read()

ValueError: I/O operation on closed file

[jgosmann]

Which seems somewhat reasonbale considering that f.read() could also be interpreted as read(f) and in that case it is indeed the value that's invalid.

[hunse]

Yeah, that makes some sense I guess. Trying to write to a read-only array in Numpy used to give a RuntimeError, I think, but now gives a ValueError; it's a bit similar to our situation here. But in nengo/network.py, we use RuntimeError a lot if the state of things isn't the way we expect. So I don't see a clear winner either way, and I'm not sure if it matters too much, as long as the message makes it clear why there's a problem.

[tbekolay]

Rebased this. Given that #781 will make a specific exception for this, I'm cool with keeping the ValueErrror (though I made a slight wording change in 316f5c9). Other than that, looks good to me, so will merge if someone can give this a quick review.

[drasmuss]

This may be a TODO for another PR, but just a pedagogical comment. Right now close doesn't really do anything, and it isn't used anywhere, so there's no reason a user would put it in their code. Which kind of defeats the purpose, that this was a backend-agnostic way to shut down a simulator. In other words, users are unlikely to put this into their code until they read up on Spinnaker and realize that there is a close method there that does useful things (which is how things worked before this PR anyway).

So it'd just be good if we could get users in the habit of gracefully closing their simulators as they learn Nengo, to make the transition between backends smoother.

If we really wanted to encourage this, we could do something like

def __del__(self):
    if not self.closed:
        warnings.warn("Simulator deleted without being closed")

That might be overkill, we could also just switch to using the context manager syntax in our examples, so that new users just learn that and don't question it.

Or if we wanted to be more magicy, we could call self.close() automatically inside __del__

[hunse]

I wasn't thinking about putting the call in __del__, which might be a good idea, but in #857 we were talking about having close() called automatically for all open simulators when the program ends. So basically the same idea. I'm in favour of something like that.

[tcstewar]

So it'd just be good if we could get users in the habit of gracefully closing their simulators as they learn Nengo, to make the transition between backends smoother.

Hmm.. I don't think the added complexity (from the perspective of a user) is worth it. Having all those sim.close() lines cluttering up the code feels wrong to me. I'd much rather go with what @hunse mentioned about having close() be automatically called when the program ends.

The main reason that having the close() function was suggested in the first place was just so that people who are using simulators that require a close() don't have to write code like this:

if isinstance(sim, nengo_spinnaker.Simulator):
    sim.close()

or

if hasattr(sim, 'close'):
    sim.close()

or

try:
    sim.close()
except AttributeError:
    pass

Having a close() function on all Simulators is meant to simplify things in this sort of scenario, but I don't think it should mean that everyone should always have to manually call close().

[tbekolay]

I don't think there's any way around forcing people to call close. If you don't call close on an open file descriptor, you will eventually get problems, and the same will eventually be true of Simulator.close. We either teach people to always do this, or we make it automatic. In previous threads about this, I was trying to advocate for it being automatic, but people wanted more control (which in the end I agree with). But with control comes responsibility, and it's on us to teach people the right way to do things. Python doesn't do this with open, but I wish it did because it would probably help debug a lot of weird issues. So I agree that we should warn and then close all open simulators when the Python process ends, if we're able to (note that we are not able to when the process is killed, only when it ends naturally or is terminated).

Only nitpick is that we should do this with atexit instead of __del__, as I am pretty sure __del__ is not guaranteed to be called in as many situations as atexit... though that's just off the top of my head, we should look into it more.

[drasmuss]

Yeah, and when we keep in mind the context manager syntax, I don't think we'll have a bunch of sim.close() "cluttering up the code". It's just

with nengo.Simulator(net) as sim:
    sim.run(1)

instead of

sim = nengo.Simulator(net)
sim.run(1)

The nice thing about __del__ is that it'd handle things properly if the user does something like

for i in range(10):
   sim = nengo.Simulator(net[i])
   sim.run(1)

(i.e. it handles the simulator getting deleted mid-script). But I'll admit that I've never used atexit so I don't really know what it can do.

[tbekolay]

Right, I see what you're saying Dan. So it should probably be both; have the check in the __del__ and then at exit, we do for sim in nengo.simulator.open_sims: del sim.

[tcstewar]

Hmm... I think I see what you're saying about consistency... my problem might be is that I'm mentally expecting it to behave like how file handles in Python behave -- no one forces me to manually call close() and I just rely on it being closed when Python quits. If I'm in a situation where I want control over closing, then I can call close(). But Python doesn't give me a warning on exit when a file hasn't been closed.

So I was expecting this to behave in the same way. Are there other examples of things in Python where you have to call close() on them, otherwise they complain, rather than silently closing themselves in their __del__ or atexit?

[hunse]

In previous threads about this, I was trying to advocate for it being automatic, but people wanted more control (which in the end I agree with).

Why can't we have both? It will be done automatically at exit or __del__, whichever comes first, but users can also force premature close by calling close() themselves.

no one forces me to manually call close() and I just rely on it being closed when Python quits. If I'm in a situation where I want control over closing, then I can call close().

This is exactly what I had in mind. Is there anyone making an argument against this? It seems to me we're all pretty much on the same page here, and I'm not quite sure what we're actually discussing.

[drasmuss]

no one forces me to manually call close() and I just rely on it being closed when Python quits

😱

This is exactly what I had in mind. Is there anyone making an argument against this? It seems to me we're all pretty much on the same page here, and I'm not quite sure what we're actually discussing.

👍

[tbekolay]

This isn't really a situation where we should look to Python. Python doesn't do any validation of function inputs either, but we dedicate a lot of our codebase to validation in order to make Nengo easier for our users.

I'm envisioning a warnings.warn if you don't manually close the simulator. You can easily suppress these warnings with warnings.filter if you don't like them. But it's pretty similar to us printing out that you have zombie threads when closing nengo_gui, IMO.

[hunse]

What is the problem with automatically closing simulators? I just don't see why we want to encourage manual closing. Certainly some backends might require it if you want to make more than one simulator (that's why this issue came up in the first place), but that's something those backends can deal with (they could even throw an exception if a second simulator is made once one already exists, or something).

[drasmuss]

I think it's based on the reliability of __del__ and atexit. To use file.close as an example, that does work OK most of the time if you just let it be closed automatically. But sometimes it doesn't work, which is why every guide to file IO says that you should really close the file yourself (https://docs.python.org/2/tutorial/inputoutput.html). So the question is just, should we let users do things wrong if they really want to? Or should we gently encourage them to use best practices?

[tbekolay]

I find it hard to dissociate the discussion of manually closing with having a close method in nengo.Simulator the first place. On the one hand, you want Nengo code to look the same regardless of the backend used, so you add a close method to the simulator. On the other hand, you still need to care about your backend to actually call the close method? The two seem to go hand-in-hand to me...

Sidenote: whoo, Nengo discussions! It's been too long 🐙 🌈

[hunse]

On the one hand, you want Nengo code to look the same regardless of the backend used, so you add a close method to the simulator.

Fair enough. I would do it context manager style then, and have run throw an error if it's called outside that context. Also, we'd need an error if a second simulator is made inside the context of another? Or while another still exists? I guess that depends if we expect simulators to allocate resources on creation or upon entering the context manager.

[tcstewar]

Hmm, it feels to me like we're veering a lot from the original purpose here. The intent of the PR was to add a dummy close() method so that people wouldn't have to do if hasattr(sim, 'close'): sim.close() whenever they're working with multiple backends. I think it's a separate discussion to decide that we're going to throw up a warning if people don't call close(). Adding in a new warning and changing how we expect people to call a Simulator (i.e. using the context approach) is a big change that affects a lot of code, and I don't see that being worth it.

[tcstewar]

Sidenote: whoo, Nengo discussions! It's been too long

Agreed! :) :) :)

[tcstewar]

On the other hand, you still need to care about your backend to actually call the close method?

When you're doing backend-specific things, yes, you need to know about your backend. So, for example, nengo_spinnaker has a limitation that only one Simulator can be open at a time. So if I try doing more than one, I get an error saying "hey, you should close the old one". Right now, that requires doing one of these if hasattr(sim, 'close'): sim.close(), if I want my code to still work with the original backend.

The intent here was to get rid of that bit of annoying code, and, since it's likely that some other backends might have a similar close() requirement, we may as well do it in a consistent manner. But I don't think that solving this problem means forcing everyone to always call close, or forcing everyone to always use a context manager. Indeed, even the nengo_spinnaker backend doesn't give any sort of warning when it goes ahead and cleans up the Simulator on atexit.

[hunse]

Adding in a new warning and changing how we expect people to call a Simulator (i.e. using the context approach) is a big change that affects a lot of code, and I don't see that being worth it.

But creating a best-practice that users always call close() in their scripts is also a big change, IMO. If that's the route that we might be going down, then I think that warrants some discussion, and we should definitely consider the context manager approach in that.

I do agree, though, that I think we can merge this PR as-is, and push that discussion until later. I think this is an improvement no matter what direction we take down the road.

[tcstewar]

But creating a best-practice that users always call close() in their scripts is also a big change, IMO

I completely agree with you. This PR was not meant to introduce such a best-practice.

If that's the route that we might be going down, then I think that warrants some discussion, and we should definitely consider the context manager approach in that. I do agree, though, that I think we can merge this PR as-is, and push that discussion until later. I think this is an improvement no matter what direction we take down the road.

Yup, I'd definitely be interested in that sort of discussion, long-term (in amongst all the other design discussions -- yay!).

[hunse]

Indeed, even the nengo_spinnaker backend doesn't give any sort of warning when it goes ahead and cleans up the Simulator on atexit.

That reminds me, the other reason this discussion came up is because we were talking about standardizing that atexit call so it's handled by core Nengo, so every backend doesn't have to go do that itself. But again, that can wait.

[hunse]

Anyway, in the course of this discussion, I looked over the PR, and it looks good to me.

[drasmuss]

Yeah I didn't take any of this as blocking this PR, this all started with "This may be a TODO for another PR...". I can make a separate issue if you want to keep the discussion out of this thread.