Threadsafe context

[jgosmann]

Not sure if we want to include this in the 2.1.0 release, but technically it's a bug fix.

This PR makes the network and config state thread-local. Without this you might get weird interactions and unpredictable behaviour between threads.

[jgosmann]

This problem came up in PR #984.

[tbekolay]

Definitely a +1 on having this in 2.1.0! I always assumed that pytest-xdist used multiple processes, not threads, so I hadn't worried about it in the past, but this is definitely the right thing to do.

A few nitpicky comments / questions:

• It would be nice if we could avoid the duplicated code (mostly CheckIndependence) in test_config, test_network, and test_threading. Perhaps a general version of CheckIndependence could be moved to the module level of test_threading and imported by the other two files? Or moved to nengo.utils.threading and all 3 import from there? Or maybe just importing the CheckIndependence from test_config in test_network is sufficient. Not sure what the best way forward is, but it'd be nice to not duplicate it.
• TheadLocalContext uses a list internally, where we previously used a deque for fear of the stack growing too large. Is there a reason for changing it? Personally I'm cool with the list change, as I don't expect these stacks to ever grow too large (and if they do, we'll have other problems) but just wondering why you changed it.
• ThreadLocalContext should probably subclass MutableSequence rather than Sequence. Also, I think the canonical location is collections.abc.MutableSequence, which might be better to use to make it obvious that it's using the abstract base class stuff.

[hunse]

This could be used for things other than our Network context, right? Should it have a more general name, like ThreadLocalList? And call the internal variable _list or something?

[hunse]

Or maybe ThreadLocalStack since this is (pretty much) a stack, and doesn't have the full functionality of a Python list.

[hunse]

It would be nice if we could avoid the duplicated code (mostly CheckIndependence) in test_config, test_network, and test_threading

I'm not sure if all these tests are necessary. The one in test_threading makes sure the object works correctly. Could the ones in test_config and test_network fail with the one in test_threading passing, in any way other than we forget to use the ThreadLocalContext for those contexts?

TheadLocalContext uses a list internally, where we previously used a deque for fear of the stack growing too large. Is there a reason for changing it? Personally I'm cool with the list change, as I don't expect these stacks to ever grow too large (and if they do, we'll have other problems) but just wondering why you changed it.

I'm not a fan of the deque here (or in the old code), since won't it just silently drop the first added element if we go over the limit? It would seem to me that we want to be checking the size every time we add an item, and raise something akin to a stack overflow error if it gets too large.

[jgosmann]

It would be nice if we could avoid the duplicated code (mostly CheckIndependence)

I have to take another look at the code to see if I can come up with something.

TheadLocalContext uses a list internally, where we previously used a deque for fear of the stack growing too large. Is there a reason for changing it?

I didn't see the reason for limiting the stack depth arbitrarily to 100. Also, as @hunse mentioned we would discard elements in the beginning when going over 100. Performance concerns seem also to not very relevant (and we're not prepending to the list which would be way more expensive).

ThreadLocalContext should probably subclass MutableSequence rather than Sequence

I don't necessarily agree. For the context we only need a stack and thus it might make sense to limit the interface to push (append) and pop. By implementing MutableSequence any position can be modified and elements can be inserted anywhere. Also, even without deriving from MutableSequence, ThreadLocalContext can still be used in place of other MutableSequences as long as only append and pop are used thanks to duck typing (and if other methods are used, ThreadLocalContext should probably not used in that spot).

[hunse]

I didn't see the reason for limiting the stack depth arbitrarily to 100

The idea behind limiting is that users will NEVER want a network stack deeper than 100, so if we end up in that situation it's because the user is doing something they don't intend to do. To me, 100 seems like a reasonable limit, but we could push it higher if people think that's too low. But I think there should be some limit, just so if a user somehow programmatically creates network stacks things don't get too out of control (though one could argue that it's very difficult for novice users to do that by accident, so it it happens it's their own damn fault).

[jgosmann]

Maybe we should make the limit an RC setting?

[hunse]

Yeah, that's definitely an option, and probably the way to go if we really think this will become limiting. But to me, that seems more trouble than it's worth. For example, if we made the limit 1000, I could never ever see any user coming up against that limit, and if someone does, then we would release a patch to change that because we'd clearly made a mistake. But just like there's a fundamental limit to the context stack of a program in terms of recursive and other functions, and that doesn't limit the things you can do with a programming language, I think we can impose a limit without limiting what our users can do. Personally, I think 100 is considerably more than any user would ever need, and for that reason I don't think it's worth adding an RC setting.

[jgosmann]

And then something like this happens. ;)

But I'm fine either way, introducing an RC setting wouldn't only be a minor effort, but probably no one ever will use it.

[hunse]

That Gangnam style example is a good point, but I would argue a) the Youtube designers should have seen that coming, since there are well over 2 billion people in the world, and it doesn't seem implausible that there would come a video that everyone would watch, never mind watch multiple times, and b) our situation is different, because the network depth you need to describe a model is going to be roughly logarithmic to the number of things in your model, so to warrant a depth of over 100, you would need well over a billion trillion things in your model, even just having two things per level, and there's nowhere near that number of neurons in the brain, so I think we'll be fine. <- run on sentence

[jgosmann]

It would be nice if we could avoid the duplicated code (mostly CheckIndependence) in test_config, test_network, and test_threading

I did some refactoring to reduce the code duplication. Also, addressed the other comments.

I'm not sure if all these tests are necessary.

Is there a downside to having those tests (except duplicated code that got reduced now)?

Could the ones in test_config and test_network fail with the one in test_threading passing, in any way other than we forget to use the ThreadLocalContext for those contexts?

One could forget to call append or pop in the right places (though that's probably also covered in other tests). I also wrote the tests in test_config and test_network first to be able to consistently reproduce the problem and have a verification that it is fixed later.

[hunse]

Is there a downside to having those tests

I think it is possible to over-test. Tests of things that are trivially true take up space, add extra work for developers who have to read and maintain them, and provide a false sense of assurance that because we're "testing" things, everything must be good. Not saying that's true of these tests per-se, but I do think one has to be careful. Also, not falling into the trap of "I wrote it so I might as well include it." I often write tests to help me get something working, but then just trash them because they end up being redundant in the final implementation. In a sense, tests that test the same thing in multiple places are their own form of code duplication.

[hunse]

All this looks good to me. I'm still not convinced that the two tests are totally necessary, but they're quite concise now so they're not really in the way either. So I'm fine with it as-is.

[tbekolay]

The test looks good to me too! I agree that it's quite possible to overtest, but I think that these tests are worth having even with that in mind. I have a few minor nitpicks on this still:

• Could we use maxsize instead of limit for the ThreadLocalStack? Feels more similar to deque's maxlen. And I think all of the instances where we pass a maxsize, we should pass it as a keyword argument, as ThreadLocalStack(100) could mean that we're initializing it with a value of 100 in the stack.
• ThreadedAssertion's docstring doesn't follow PEP-257. It should have a one-line summary and then a body separated by a blank line and then the closing """ should be on a separate line.
• I think ideally the exception in ThreadLocalStack would be a subclass of NengoException, but there isn't a great exception that currently exists. Maybe we could generalize NetworkContextError to StackError?

[tbekolay]

Looks good now 😄 🌈 Merging!

[jgosmann]

Oh, I didn't change the exception yet ... should I still do that?

[tbekolay]

Nah, it's ok for now. I think it'll be good to see how people interact with exceptions for a while.