Inspective parsing

[jgosmann]

Motivation and context:
This PR changes the parsing so that it uses the locals and globals dictionaries of the calling function to resolve names. This allows to access all variables defined in the Python code that are in scope. That can be helpful to specify vocabularies or in the future solvers for vocab translation (see #62). It also makes it so that exactly the same names are used to access things in the Python code and in the action rules (previously the network name had to be left out in the action rules). Moreover, this completely eliminates the need to assign SPA modules/networks as attributes.

There might be a slight issue: We allow to provide Semantic Pointer names, but such a name can be hidden by the something in the Python namespace. There isn't really a good way to figure out whether a name should refer to a Semantic Pointer or a Python object as long as we do not introduce a special syntax for Semantic Pointers.

Also, predefined built-ins (dot, reinterpret, and translate) will hide any other object with the same name. But in pretty much any programming language there are reserved names that you cannot use for your own variable names, so I think this should be fine.

Interactions with other PRs:
Yes, this PR solves some issues with the implementation of #62 which requires access to solvers (which can be arbitrary Python classes) in the action rules.

How has this been tested?
Tests have been changed to reflect the new name resolution. No new tests are required as this does not really add a new feature, but just changes how existing features are used.

How long should this take to review?

• Lengthy (more than 150 lines changed or changes are complicated)

Most of the changes are adjusting the syntax to the new name resolution rules. The actual core changes are pretty confined.

Where should a reviewer start?

It probably makes sense to reviews this PR commit by commit as each commit leaves the code in a state that should pass tests. The middle commit will still be pretty huge, though. For that one it is probably best to look at the core changes in ast.py, actions.py, and network.py (not necessarily in that order) and than move on to all the adjustments in the unit tests.

Types of changes:

• Breaking change (fix or feature that causes existing functionality to change)

Checklist:

• I have read the CONTRIBUTING.rst document.
• [n/a] I have updated the documentation accordingly.
• I have included a changelog entry.
• [n/a] I have added tests to cover my changes.
• All new and existing tests passed.

[jgosmann]

Urgs, this produces a whole bunch of pyflakes warnings because the attributes are now variables that are unused (it doesn't know that they get used in the action rules). Not sure what to do about this, potential solutions:

• Deactivate the check for the test files
• Assign everything as an attribute again
• ...?

I'll have to think about this ...

[jgosmann]

Added commits to fix various problems including the pyflakes warnings. I decided to go back to assign things to attributes where required. I think it makes sense because those things are part of the respective networks and doesn't require to disable a pyflakes warning that can be helpful in other instances. Also, it is less boilerplate than adding assert statements to trick pyflakes into thinking that the variables are used.

Users may make there on decisions how to handle this in their projects (they might not even use pyflakes).

[Seanny123]

Would this control-flow make more sense to me if I knew more about compilers?

[Seanny123]

Could you add a comment about the function of locals here? Why does it default to None? How does it control iterating through frames?

[jgosmann]

I guess this could use some sort of comment because locals_ can be one of three things:

• a dictionary,
• None in which case the locals of the calling function will be used (what is what a user calling that function usually wants),
• or an integer that defines of how many frames up the locals should be grabbed (this is used because other user-facing functions call this function and it it is easier to just increase this number by one than duplicating code to grabbing the locals dictionary of the calling function)

[Seanny123]

That makes sense. A comment would be great!

[Seanny123]

Nitpick: would it be better to name this local_vars or something else? I understand it's called locals_ because locals is a reserved keyword, but a trailing underscore feels a bit weird?

[jgosmann]

Appending an underscore seems like a pretty common think to do in these cases (especially as locals_ is intended to be locals()). I'm pretty sure I have seen similar things in other code bases. But I'm open to changing it if that's the consensus.

I feel @tbekolay might dislike the underscore too?

[Seanny123]

There might be a slight issue: We allow to provide Semantic Pointer names, but such a name can be hidden by the something in the Python namespace. There isn't really a good way to figure out whether a name should refer to a Semantic Pointer or a Python object as long as we do not introduce a special syntax for Semantic Pointers.

Is it possible to throw an error in the case of a name conflict? This feels like something I would appreciate having an error for.

[Seanny123]

This code is nice and seems to address a lot of usability problems while removing a lot of boilerplate code. However, I would like a bit of the code explained to me before I approve. 😄

[jgosmann]

Is it possible to throw an error in the case of a name conflict? This feels like something I would appreciate having an error for.

We can't easily figure out which semantic pointers are defined a parse time I believe (they might be added on-the-fly for non-strict vocabularies). I'm thinking whether it might make sense to require semantic pointer names to be given as strings to avoid such name conflicts. Slightly awkward maybe because of the nested quotes (but at least Python supports two types of quotes) and the additional two character to type.

However, I would like a bit of the code explained to me before I approve.

That's a fair request and why I want these PR reviews. :)

[jgosmann]

Added a commit to add some documentation for locals_ and remove the None variant (it is not really needed as the integer can be directly provided as a default value and simplifies things).

I think what to do about semantic pointer names will require more discussion.

Before changing the name of locals_ I would like to here more opinions.

Are there any other parts where you like me to add some more documentation?

[Seanny123]

I'm okay with the name locals_ upon further consideration and really appreciate it's clarification.

[jgosmann]

Pinging @tcstewar, not so much for a full code review, but more for an opinion on the general direction things move to with this PR.

[jgosmann]

Pinging @tcstewar again.

Also, I realized that with this PR we should be able to get rid of the requirement to use spa.Network instead of nengo.Network for networks containing SPA modules, though it would still be helpful for configuring modules.

[tcstewar]

So, after playing with this a bit, I'm more and more convinced that this is the correct way to go.

My initial hesitation was that I love doing Python black magic, and I often go overboard with it, and I'm trying to do that less, and this is definitely Python black magic. But, as @jgosmann pointed out, the current system is also black magic, and this new system's black magic is actually a lot more describable, due to getting rid of the weird "oh, and you have to assign it to model" thing.

I think the biggest reason I really like this is that every time I've taught the nengo.spa stuff, that weird model.whatever thing is always something that I stumble on and don't have a good story for, other than "you just have to do it that way and don't ask questions about it". Which students are usually totally fine with, but I find it disconcerting....

[tcstewar]

One interesting consequence of this approach that I actually think is correct, but surprising, is this error:

D = 32
model = spa.Network()
with model:
    memory = spa.State(D, feedback=1, feedback_synapse=0.01)
    spa.Actions([
        'dot(memory, A) --> memory=B',
        'dot(memory, B) --> memory=C',
        'dot(memory, C) --> memory=D',
        'dot(memory, D) --> memory=E',
        'dot(memory, E) --> memory=A',
        ])

Traceback (most recent call last):
  File "c:\users\terry\documents\github\nengo_gui\nengo_gui\page.py", line 220, in execute
    exec(compiled, code_locals)
  File "<nengo_gui_compiled>", line 25, in <module>
  File "c:\users\terry\documents\github\nengo_spa\nengo_spa\actions.py", line 225, in __init__
    self.bg, self.thalamus, self.connstructed = self.build()
  File "c:\users\terry\documents\github\nengo_spa\nengo_spa\actions.py", line 286, in build
    action.infer_types(root_network, None)
  File "c:\users\terry\documents\github\nengo_spa\nengo_spa\ast.py", line 1104, in infer_types
    self.effects.infer_types(root_network, None)
  File "c:\users\terry\documents\github\nengo_spa\nengo_spa\ast.py", line 1016, in infer_types
    e.infer_types(root_network, context_type)
  File "c:\users\terry\documents\github\nengo_spa\nengo_spa\ast.py", line 974, in infer_types
    self.source.type, self.sink.type, self))
nengo_spa.exceptions.SpaTypeError: Cannot assign TScalar to TVocabulary<32-dimensional vocab at 0x2a920b282e8> in 'memory = 32'

This is, I think, I perfectly legitimate error, in that I am using D to mean two completely different things -- the dimensionality and the semantic pointer D. So there might be other situations like that lying around, but there shouldn't be too many of them.....

[tcstewar]

Also, I initially thought that it would be great if this worked, but now I'm not so sure:

vocab = spa.Vocabulary(32, strict=False)
E = vocab.parse('E')

model = spa.Network()
with model:
    memory = spa.State(vocab, feedback=1, feedback_synapse=0.01)
    spa.Actions([
        'dot(memory, A) --> memory=B',
        'dot(memory, B) --> memory=C',
        'dot(memory, C) --> memory=D',
        'dot(memory, D) --> memory=E',
        'dot(memory, E) --> memory=A',
        ])

The problem with this is that I've now pinned E to be in a particular vocabulary, and things could start going horribly wrong if we start mixing vocabularies at this point. So I think I now believe that this should not work (which is the current behaviour).

[jgosmann]

It seems reasonable to me to say, everything that starts with a capital and is not a known variable name, will be treated as a semantic pointer name. (Alternatively, everything with a starting with a capital could be treated as a semantic pointer name which could hide variables.)

If you have a variable, but want to assign a Semantic Pointer with that name, the following workaround currently works:

import nengo_spa as spa
from nengo_spa.ast import Symbol

D = 32
model = spa.Network()
with model:
    memory = spa.State(D, feedback=1, feedback_synapse=0.01)
    spa.Actions([
        'dot(memory, A) --> memory=B',
        'dot(memory, B) --> memory=C',
        'dot(memory, C) --> memory=Symbol("D")',
        'dot(memory, Symbol("D")) --> memory=E',
        'dot(memory, E) --> memory=A',
        ])

I think we should have a workaround like that and have it documented. The questions is whether to make it more intuitive (e.g., import from nengo_spa instead of the internal nengo_spa.ast module; use a different name than Symbol which makes as a name makes sense in the context of the AST, but maybe not so much to a user). It would also be possible to change the syntax to something like symbol.D to avoid the quotes, but that is darker magic to me and seems like a weirder syntax.

Furthermore, I contemplating about the “built-ins” (dot, reinterpret, and translate). These currently hide any variable with such a name. Not sure if that's better than having the variables potentially hide the built-ins. A way to completely get around this issue is to not have built-ins and the user would have to import these functions from nengo_spa.

[jgosmann]

Note that you cannot use the built-ins outside of the action rules (at least not in the way you would expect, dot is not numpy.dot).