Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attribute assignment safeguards for NestModule and SynapseCollection #2220

Merged
merged 10 commits into from
Dec 7, 2021

Conversation

Helveg
Copy link
Contributor

@Helveg Helveg commented Nov 26, 2021

  • Settings unknown attributes on nest module raises an AttributError
  • Fixed an always-true condition that allowed setting unknown attributes on SynapseCollection.

Closes #2219.

Just 1 remark: while the network or synapse collection is empty, you can set unknown attributes. Any idea why this was implemented, should it remain that way?

@heplesser heplesser added I: User Interface Users may need to change their code due to changes in function calls S: High Should be handled next T: Bug Wrong statements in the code or documentation labels Nov 26, 2021
@heplesser heplesser added this to the NEST 3.2 milestone Nov 26, 2021
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Helveg Thanks!

  • This change seems too drastic, all Pynest tests seem to fail now ;).
  • How does this address the problem that unknown synapse collection objects are not detected?
  • Please add a regression test, both for kernel and synapse attributes.

pynest/nest/__init__.py Show resolved Hide resolved
pynest/nest/__init__.py Show resolved Hide resolved
@heplesser
Copy link
Contributor

Just 1 remark: while the network or synapse collection is empty, you can set unknown attributes. Any idea why this was implemented, should it remain that way?

I cannot think of any reason for this behavior. One technical aspect might be that the detection of unknown parameter names for node collections largely happens in SetStatus() in the kernel, and for an empty collection we have nothing to call that on. @stinebuu @hakonsbm Any comments?

@Helveg
Copy link
Contributor Author

Helveg commented Nov 26, 2021

How does this address the problem that unknown synapse collection objects are not detected?

Where can I find a description of this problem? I thought the problems were related to attribute assignment on nest and SynapseCollection?

@heplesser
Copy link
Contributor

How does this address the problem that unknown synapse collection objects are not detected?

Where can I find a description of this problem? I thought the problems were related to attribute assignment on nest and SynapseCollection?

Sorry, my comment was unclear. What I meant is: How does this change ensure that

c = nest.GetConnections()
c.gewicht = 10

will raise an exception because gewicht is not a known synapse parameters. It should have been "parameter" or "attribute", not "object" in my original question.

@Helveg
Copy link
Contributor Author

Helveg commented Nov 26, 2021

There already was a __setattr__ implementation that defers to SynapseCollection.set, which goes into SLI and errors graciously if the key isn't known, it just wasn't being called. The problem was that the condition to defer to .set was always false, and instead super().__setattr__(attr, value) was always called which would just set the attribute the regular Python way. You can see in e87ae1c how I remedied that.

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Helveg Thanks for the fixes! Could you improve comments a little, see inline remarks?

pynest/nest/__init__.py Show resolved Hide resolved
testsuite/pytests/test_get_set.py Show resolved Hide resolved
testsuite/pytests/test_get_set.py Show resolved Hide resolved
@hakonsbm
Copy link
Contributor

Just 1 remark: while the network or synapse collection is empty, you can set unknown attributes. Any idea why this was implemented, should it remain that way?

I cannot think of any reason for this behavior. One technical aspect might be that the detection of unknown parameter names for node collections largely happens in SetStatus() in the kernel, and for an empty collection we have nothing to call that on. @stinebuu @hakonsbm Any comments?

@heplesser Yes, but the behaviour isn't consistent for NodeCollections and SynapseCollections.

For empty NodeCollections, trying to get a non-existent attribute results in an AttributeError, while trying to set an attribute will just return None without setting anything.

An empty SynapseCollection will return an empty tuple when trying to get an unknown attribute, while setting an attribute will set the attribute on the SynapseCollection itself. Interestingly, setting a valid attribute, like weight, on a non-empty SynapseCollection will also set it as an attribute of the SynapseCollection, not as a property of the connections.

Copy link
Contributor

@stinebuu stinebuu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, I only have a small comment on a comment.

Regarding the different behavior for NodeCollection and SynapseCollection, I cannot remember why there is a difference. Maybe something we should look at further in another issue?

testsuite/pytests/test_get_set.py Outdated Show resolved Hide resolved
Co-authored-by: Stine Brekke Vennemo <stine.vennemo@gmail.com>
Copy link
Contributor

@stinebuu stinebuu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@heplesser heplesser merged commit 054b355 into nest:master Dec 7, 2021
@Helveg Helveg deleted the attr-safeguard branch December 7, 2021 10:26
@jougs
Copy link
Contributor

jougs commented Dec 7, 2021

@Helveg: The merge of this PR leads to the following error on Read the Docs.

Traceback (most recent call last):
  File "/home/docs/checkouts/readthedocs.org/user_builds/nest-simulator/envs/latest/lib/python3.8/site-packages/sphinx/cmd/build.py", line 276, in build_main
    app = Sphinx(args.sourcedir, args.confdir, args.outputdir,
  File "/home/docs/checkouts/readthedocs.org/user_builds/nest-simulator/envs/latest/lib/python3.8/site-packages/sphinx/application.py", line 216, in __init__
    self.config = Config.read(self.confdir, confoverrides or {}, self.tags)
  File "/home/docs/checkouts/readthedocs.org/user_builds/nest-simulator/envs/latest/lib/python3.8/site-packages/sphinx/config.py", line 173, in read
    namespace = eval_config_file(filename, tags)
  File "/home/docs/checkouts/readthedocs.org/user_builds/nest-simulator/envs/latest/lib/python3.8/site-packages/sphinx/config.py", line 342, in eval_config_file
    raise ConfigError(msg % traceback.format_exc()) from exc
sphinx.errors.ConfigError: There is a programmable error in your configuration file:

Traceback (most recent call last):
  File "/home/docs/checkouts/readthedocs.org/user_builds/nest-simulator/envs/latest/lib/python3.8/site-packages/sphinx/config.py", line 329, in eval_config_file
    exec(code, namespace)
  File "/home/docs/checkouts/readthedocs.org/user_builds/nest-simulator/checkouts/latest/doc/userdoc/conf.py", line 86, in <module>
    nest.NestModule = type(nest)
  File "/home/docs/checkouts/readthedocs.org/user_builds/nest-simulator/checkouts/latest/pynest/nest/__init__.py", line 423, in _setattr_error
    raise err from None
AttributeError: can't set attribute 'NestModule' on module 'nest'

@steffengraber, @jessica-mitchell, @terhorstd

@terhorstd
Copy link
Contributor

This breaks the documentation generation: https://readthedocs.org/api/v2/build/15469804.txt
We're looking into it.

@terhorstd
Copy link
Contributor

This was also one of the Python trickery nest.NestModule = type(nest) in Sphinx's conf.py, that is now not possible anymore. Where would this best to be fixed? Here or in conf.py?

@Helveg
Copy link
Contributor Author

Helveg commented Dec 7, 2021

  1. Check if the trickery is still required, the line may just be removed (check if the KernelAttribute doc page is still generated correctly)
  2. Change it to vars(nest)["NestModule"] = type(nest)

@stinebuu stinebuu added this to In progress in PyNEST via automation Dec 17, 2021
@stinebuu stinebuu moved this from In progress to Done in PyNEST Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: User Interface Users may need to change their code due to changes in function calls S: High Should be handled next T: Bug Wrong statements in the code or documentation
Projects
PyNEST
  
Done
Development

Successfully merging this pull request may close these issues.

Prohibit setting of unknown attributes on nest kernel and synapses
6 participants