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

Fix handling of PyNEST and SLI commandline arguments #840

Merged
merged 7 commits into from Mar 7, 2018

Conversation

@jougs
Copy link
Contributor

@jougs jougs commented Oct 13, 2017

In particular, this makes the following changes:

  • Replace the the command line argument --sli-debug by --debug only right before loading the pynestkernel
  • Make all given command line arguments available in the statusdict
  • introduce --sli-debug to avoid confusion with Python's --debug command line argument
  • Remove mutation of argv by PyNEST

This fixes #779 and fixes #784.

jougs added 2 commits Oct 13, 2017
* Replace the the command line argument --sli-debug by --debug only right before loading the pynestkernel
* Make all given commandline arguments available in the statusdict
* introduce --sli-debug to avoid confusion with Python's --debug argument
Copy link
Contributor

@heplesser heplesser left a comment

@jougs Thanks, this looks pretty good! I just have a few small comments in the code. Is there any sensible way to actually test that this all works as part of our test suite?

@@ -25,6 +25,7 @@

import sys
import os
from copy import copy

This comment has been minimized.

@heplesser

heplesser Oct 14, 2017
Contributor

For consistency with other imports, I would suggest import copy here and copy.copy() below.

if argv.count("--quiet"):
quiet = True
argv.remove("--quiet")
nest_argv = copy(argv)

This comment has been minimized.

@heplesser

heplesser Oct 14, 2017
Contributor

Please add a comment to explain why you copy and manipulate the argument list.

@jougs
Copy link
Contributor Author

@jougs jougs commented Feb 6, 2018

@heplesser, @apeyser: I've updated the PR to reply to the review comments. Can you please have another look? Thanks!

@terhorstd terhorstd added this to the NEST 2.16 milestone Mar 5, 2018
Copy link
Contributor

@apeyser apeyser left a comment

Some questions regarding python and SLI

# handle NEST's arguments here and pass it a modified copy, while
# we leave the original list unchanged for further use by the user
# or other modules.
nest_argv = copy.copy(argv)

This comment has been minimized.

@apeyser

apeyser Mar 5, 2018
Contributor

nest_argv = argv[:]

is clearer for me.

This comment has been minimized.

@jougs

jougs Mar 5, 2018
Author Contributor

Done.

nest_argv.remove("--debug")
if "--sli-debug" in nest_argv:
nest_argv.remove("--sli-debug")
nest_argv.append("--debug")

This comment has been minimized.

@apeyser

apeyser Mar 5, 2018
Contributor

Does command line ordering matter? If so, this re-orders operations.

This comment has been minimized.

@jougs

jougs Mar 5, 2018
Author Contributor

I don't see any reason why it should.

nest_argv.remove("--sli-debug")
nest_argv.append("--debug")

initialized |= engine.init(nest_argv, __path__[0])

This comment has been minimized.

@apeyser

apeyser Mar 5, 2018
Contributor

Why are we using |= here? Is this really a bit map, or is this intended to be

initializedp = engine.init(nest_argv, __path__[0])
if not initialized: initialized = initializedp

This comment has been minimized.

@jougs

jougs Mar 5, 2018
Author Contributor

I've removed the | from the |= as I did not see a good reason for having it.

@@ -289,7 +289,8 @@ def setUp(self):

# build
self.pop = nest.Create('iaf_psc_alpha', 10)
self.local_nodes = nest.GetNodes([0], {'model': 'iaf_psc_alpha'}, True)[0]
local_nodes = nest.GetNodes([0], {'model': 'iaf_psc_alpha'}, True)
self.local_nodes = local_nodes[0]
self.spike_detector = nest.Create('spike_detector')

This comment has been minimized.

@apeyser

apeyser Mar 5, 2018
Contributor

Why is this in this pull request?

This comment has been minimized.

@jougs

jougs Mar 5, 2018
Author Contributor

Because PEP8 requested me to make the line shorter a dnI tried to do it in a nicer way than just breaking it.

@@ -176,23 +176,20 @@ cdef class NESTEngine(object):
raise NESTError("argv can't be empty")

# Create c-style argv arguments from sys.argv
cdef char* arg0 = "pynest\0"

This comment has been minimized.

@apeyser

apeyser Mar 5, 2018
Contributor

So, is the crazy SLI requirement gone?

This comment has been minimized.

@jougs

jougs Mar 5, 2018
Author Contributor

Yes.

for i, argvi in enumerate(argv_bytes):
argv_chars_off[i] = argvi # c-string ref extracted
argv_chars[i] = argvi # c-string ref extracted

This comment has been minimized.

@apeyser

apeyser Mar 5, 2018
Contributor

Does

argv_chars = [<char*> argvi in argv_bytes]

work? That's simpler
Or even maybe

argv_chars[:] = argv_bytes[:]

This comment has been minimized.

@jougs

jougs Mar 5, 2018
Author Contributor

Unfortunately this does not work, so I've left it unchanged. Here's the error messages for reference.

Error compiling Cython file:
------------------------------------------------------------
...
            # Need to keep a reference to encoded bytes issue #377
            # argv_bytes = [byte...] which internally holds a reference
            # to the c string in argv_char = [c-string... NULL]
            # the `byte` is the utf-8 encoding of sys.argv[...]
            argv_bytes = [argvi.encode() for argvi in argv]
            argv_chars[:] = argv_bytes[:]
                                     ^
------------------------------------------------------------

/home/jochen/work/src/nest-simulator/pynest/pynestkernel.pyx:191:38: Cannot convert Python object to 'char **'

Error compiling Cython file:
------------------------------------------------------------
...
            # Need to keep a reference to encoded bytes issue #377
            # argv_bytes = [byte...] which internally holds a reference
            # to the c string in argv_char = [c-string... NULL]
            # the `byte` is the utf-8 encoding of sys.argv[...]
            argv_bytes = [argvi.encode() for argvi in argv]
            argv_chars[:] = argv_bytes[:]
                                     ^
------------------------------------------------------------

/home/jochen/work/src/nest-simulator/pynest/pynestkernel.pyx:191:38: Storing unsafe C derivative of temporary Python reference

Copy link
Contributor

@apeyser apeyser left a comment

Edit with better comments

@jougs
Copy link
Contributor Author

@jougs jougs commented Mar 5, 2018

@apeyser: I've addressed all your comments in the most recent commit and in the replies above.

…x-commandline-arguments
@apeyser
apeyser approved these changes Mar 5, 2018
Copy link
Contributor

@apeyser apeyser left a comment

Agreed.

@heplesser heplesser merged commit 47846f5 into nest:master Mar 7, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.