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 of issue 246 - Make help and helpdesk more robust #688

Merged
merged 24 commits into from Jun 9, 2017

Conversation

@steffengraber
Copy link
Collaborator

@steffengraber steffengraber commented Mar 22, 2017

With this pull request, NEST uses only Python for the help system if you are on the Python level.
Therefore it doesn‘t matter if it is compiled with MPI or not.

In jupyter notebooks nest.help(obj) open a modal window with the help text.
With nest.help(obj, pager), you can open a texeditor like gedit from a notebook. If you use an terminaleditor (vi, nano, less, …) as pager, again the modal window open up.

If you use nest.helpdesk() in a notebook a new tab is created.

This pull request is a fix for issue #246.

iptnk = cfg['IPKernelApp']['parent_appname']
except NameError:
iptnk = ''
# test if jupyther notebook

This comment has been minimized.

@heplesser

heplesser Mar 23, 2017
Contributor

Jupyter

@@ -400,6 +404,124 @@ def broadcast(item, length, allowed_types, name="item"):
return item


def check_nb():
"""Check if is notebook or not
"""

This comment has been minimized.

@heplesser

heplesser Mar 23, 2017
Contributor

Maybe better

"""Return true if called from a Jupyter notebook."""
iptnk = ''
# test if jupyther notebook
jpnk = re.findall(r'.*jupyter.*', os.environ['_'])
ipjptk = re.findall(r'.*ipython.*', os.environ['_'])

This comment has been minimized.

@heplesser

heplesser Mar 23, 2017
Contributor

When calling this from a plain ipython console, it returns a match (Python 2.7.12). So the function would return True also when called from the ipython console, not just from a notebook. Is that intentional?

elif ipjptk:
return True
else:
return False

This comment has been minimized.

@heplesser

heplesser Mar 23, 2017
Contributor

You could simplify this to (but see comment above)

return iptnk == 'ipython-notebook' or re.search(r'ipython|jupyter', os.environ['_'])
});
}
);
""" % (objname, hlptxt)))

This comment has been minimized.

@heplesser

heplesser Mar 23, 2017
Contributor

% substitution is deprecated in Python 3, .format() should be used instead.

return False


def open_window(objname, hlptxt):

This comment has been minimized.

@heplesser

heplesser Mar 23, 2017
Contributor

Maybe show_help_in_modal_window() would be a better name? Should this be marked as private with a _?

""" % (objname, hlptxt)))


def pdoc(hlpobj, pager):

This comment has been minimized.

@heplesser

heplesser Mar 23, 2017
Contributor

Maybe call it show_help_with_pager() would be a better name, and maybe also this one marked private?

@heplesser
Copy link
Contributor

@heplesser heplesser commented Mar 23, 2017

@steffengraber I am testing now and here are some experiences:

  • If NEST_INSTALL_DIR is not defined, nest.help() crashes. It would be better to provide the user with more guidance, something like:
if 'NEST_INSTALL_DIR' not in os.environ:
    print('NEST help needs to know where NEST is installed. Please source nest_vars.sh or defined NEST_INSTALL_DIR manually.')
    return
  • Instead of os.environ['NEST_INSTALL_DIR'] + "/share/doc/nest/help/" it is better to use os.path.join()
  • Is it best to use less as default parser, or would should we use the even more default more? no matter what we use as default, we should check that it exists and if not, either use cat or read the help file into Python and print() it.
  • We should avoid the low-level subprocess.Popen() and rather use the high-level functions. Since we need to maintain compatibility back to 2.7, we should use the Old-style high-level API.
  • I got this error: FileNotFoundError: [Errno 2] No such file or directory: 'less -S', which is probably because subprocess does not like options combined with the command name in one string (calling from plain python or ipython); the -S comes from .nestrc, if I remove it there, things work
  • nest.help('abs') works from notebook
  • `nest.helpdesk() does nothing when called from plain python or ipython or notebook
Copy link

@janhahne janhahne left a comment

Calling nest.helpdesk() works fine at my workstation. When I call nest.help('iaf_psc_alpha') or any NEST command I end up with the following error

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../nest-fix246.install/lib64/python2.7/site-packages/nest/lib/hl_api_helper.py", line 234, in stack_checker_func
    return f(*args, **kwargs)
  File ".../nest-fix246.install/lib64/python2.7/site-packages/nest/lib/hl_api_info.py", line 90, in help
    pdoc(hlpobj, pager)
  File ".../nest-fix246.install/lib64/python2.7/site-packages/nest/lib/hl_api_helper.py", line 521, in pdoc
    proc = subprocess.Popen([pager, objf])
  File "/usr/lib64/python2.7/subprocess.py", line 709, in __init__
    errread, errwrite)
  File "/usr/lib64/python2.7/subprocess.py", line 1326, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory

Am I using it wrong, is this related to something already mentioned by @heplesser or another bug?

@steffengraber
Copy link
Collaborator Author

@steffengraber steffengraber commented May 4, 2017

@janhahne Sorry for the late response.
How do you compile nest? With MPI on or off?
Under which operating system?

@janhahne
Copy link

@janhahne janhahne commented May 4, 2017

@steffengraber I compiled nest with MPI on and my operating system is openSUSE 13.1. The problem still persist with your recent commits. Just to prevent any error on my side: nest.help('iaf_psc_alpha') should be a correct use of nest.help, right?

@steffengraber
Copy link
Collaborator Author

@steffengraber steffengraber commented May 4, 2017

@janhahne yes, nest.help('iaf_psc_alpha') is right.
Is python installed, and what version?

@janhahne
Copy link

@janhahne janhahne commented May 4, 2017

@steffengraber I am using python 2.7.6 and usually pynest works fine for me

@steffengraber
Copy link
Collaborator Author

@steffengraber steffengraber commented May 4, 2017

@janhahne with Python older than 2.7.8 the help generation did not work correctly. Is it possible for you to update and test ist again?

@janhahne
Copy link

@janhahne janhahne commented May 4, 2017

@steffengraber I updated python to 2.7.13 and did a new clean build, but the error remains the same. I also noticed that if I enter any object that does not exist (as for example nest.help('testtest')) nothing happens, so there is no output and no exception.

@steffengraber
Copy link
Collaborator Author

@steffengraber steffengraber commented May 4, 2017

@janhahne Could you please try nest.help('iaf_psc_alpha', 'less').

@janhahne
Copy link

@janhahne janhahne commented May 4, 2017

@steffengraber nest.help('iaf_psc_alpha', 'less') works fine. The corresponding documentation is displayed and no error occurs.

Copy link
Contributor

@heplesser heplesser left a comment

It works for me now---thanks! There are just a few little things to clean up, see detailed comments.


sr("/helpdesk << /command (%s) >> SetOptions" % browser)
sr("helpdesk")
url = os.path.join(os.environ['NEST_DOC_DIR'] + "/help", "helpindex.html")

This comment has been minimized.

@heplesser

heplesser May 26, 2017
Contributor

Provide a useful error message if NEST_DOC_DIR is undefined, similar as for missing NEST_INSTALL_DIR.

This comment has been minimized.

@heplesser

heplesser Jun 1, 2017
Contributor

Here you mix os.path.join() with string addition to build a URL. I think since we want a URL, we should not use os.path.join() here.

sr("helpdesk")
url = os.path.join(os.environ['NEST_DOC_DIR'] + "/help", "helpindex.html")
if sys.platform[:3] == "win":
os.startfile(url)

This comment has been minimized.

@heplesser

heplesser May 26, 2017
Contributor

Why don't you use webbrowser also under Windows?

if sys.platform[:3] == "win":
os.startfile(url)
if sys.platform[:3] == "dar":
webbrowser.get('safari').open_new(url)

This comment has been minimized.

@heplesser

heplesser May 26, 2017
Contributor

Why not webbrowser.open_new(url) also here? Ok, due to this problem in OSX we need to ask for the browser explicitly under macOS are present (10.12.5). Worse, a failing webbrowser.open_new(url) does not even raise a Python exception but returns happily. So I think we need to keep this, but i would be good to document why we do this.

helpdir = os.path.join(os.environ['NEST_INSTALL_DIR'], "share", "doc",
"nest", "help")
objname = hlpobj + '.hlp'
# @todo more pager

This comment has been minimized.

@heplesser

heplesser May 26, 2017
Contributor

Isn't the pager list pretty complete?

browser : str, optional
Name of the browser to use
The system default browser.
/helpdesk << /command (firefox) >> SetOptions in ~/.nestrc is now obsolete

This comment has been minimized.

@heplesser

heplesser May 26, 2017
Contributor

I would drop this line of documentation, since it will confuse most users. The nestrc line is still relevant for the SLI level, so I'd leave it in nestrc.

@heplesser
Copy link
Contributor

@heplesser heplesser commented May 31, 2017

@steffengraber PEP8 was still not entirely happy:

MSGBLD0195: [PEP8] pynest/nest/lib/hl_api_info.py:74:56: W291 trailing whitespace
MSGBLD0195: [PEP8] pynest/nest/lib/hl_api_info.py:80:59: W291 trailing whitespace

Otherwise, this looks fine to me.

@janhahne Do you also explicitly approve?

@steffengraber
Copy link
Collaborator Author

@steffengraber steffengraber commented May 31, 2017

@heplesser
@janhane found 2 more issues I have to address:

  • exception needed fo something like nest.help('testtest')
  • nest.help('iaf_psc_alpha', 'less') works perfect for @janhahne but not nest.help('iaf_psc_alpha').
Copy link
Contributor

@heplesser heplesser left a comment

I found a few more details ...

pager = 'less'
rc.close()

# check if .netsrc exist

This comment has been minimized.

@heplesser

heplesser Jun 1, 2017
Contributor

.nestrc

rc.close()

# check if .netsrc exist
rc_file = os.environ['HOME'] + '/.nestrc'

This comment has been minimized.

@heplesser

heplesser Jun 1, 2017
Contributor

You should use os.path.join(os.environ['HOME'], '.nestrc') for portable path construction; applies in other places as well.

rc_file = os.environ['HOME'] + '/.nestrc'
if os.path.isfile(rc_file):
# open ~/.nestrc
rc = open(os.environ['HOME'] + '/.nestrc', 'r')

This comment has been minimized.

@heplesser

heplesser Jun 1, 2017
Contributor

rc = open(rc_file, 'r')

# open ~/.nestrc
rc = open(os.environ['HOME'] + '/.nestrc', 'r')
for line in rc:
rctst = re.match(r'^\s?%', line)

This comment has been minimized.

@heplesser

heplesser Jun 1, 2017
Contributor

Could you add a comment explaining/illustrating briefly what this RE looks for; that will make the code more maintainable. Applies also to other REs.

I think it would also be helpful if you could explain what this entire loop is trying to do---I cannot quite get it just from the code.


sr("/helpdesk << /command (%s) >> SetOptions" % browser)
sr("helpdesk")
url = os.path.join(os.environ['NEST_DOC_DIR'] + "/help", "helpindex.html")

This comment has been minimized.

@heplesser

heplesser Jun 1, 2017
Contributor

Here you mix os.path.join() with string addition to build a URL. I think since we want a URL, we should not use os.path.join() here.

Copy link

@janhahne janhahne left a comment

@steffengraber Thank you for addressing my issues! Now everything works as it should for me 👍

subprocess.call([pager, objf])
fhlp.close()
break
if hlperror == True:

This comment has been minimized.

@heplesser

heplesser Jun 2, 2017
Contributor

if hlperror for tidier code and to make PEP8 happy.

@steffengraber
Copy link
Collaborator Author

@steffengraber steffengraber commented Jun 8, 2017

@heplesser I added some documentation and PEP8 is happy now.

@heplesser
Copy link
Contributor

@heplesser heplesser commented Jun 9, 2017

Release notes: Improved help in PyNEST

  • Help will work even if compiled with MPI
  • Help will work in Jupyter Notebooks
Copy link
Contributor

@heplesser heplesser left a comment

@steffengraber Thanks for the fixes, just two little details about path construction left to make it entirely pythonic and even windows-safe.


sr("/helpdesk << /command (%s) >> SetOptions" % browser)
sr("helpdesk")
url = os.path.join(os.environ['NEST_DOC_DIR'] + "/help", "helpindex.html")

This comment has been minimized.

@heplesser

heplesser Jun 9, 2017
Contributor

I still have some issues with this line:

  1. This is not a URL, but the name of a local file to be opened by a browser, so the variable name should be helpfile or similar, not url.
  2. Instead of mixing string concatenation and os.path.join(), just use os.path.join():
helpfile = os.path.join(os.environ['NEST_DOC_DIR'], 'help', 'helpindex.html')
# reading ~/.nestrc lookink for pager to use.
if pager is None:
# check if .netsrc exist
rc_file = os.environ['HOME'] + '/.nestrc'

This comment has been minimized.

@heplesser

heplesser Jun 9, 2017
Contributor

This should be

rc_file = os.path.join(os.environ['HOME'], '.nestrc')
Minor Fix.
@heplesser heplesser merged commit d728947 into nest:master Jun 9, 2017
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.

None yet

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