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 PyNest help and helpdesk commands #1926

Merged
merged 21 commits into from
Mar 18, 2021
Merged

Conversation

jougs
Copy link
Contributor

@jougs jougs commented Feb 17, 2021

This fixes #1904 by making helpdesk() open a link to the Sphinx-generated HTML help index page and the help() function operate on the reStructuredText files extracted from the models. I've also simplified the handling of the pager by just using the one provided by pydoc instead of from SLI. All pertaining documentation files have been updated.

This PR is based on the branch behind #1843 and should thus not be merged before that. For the same reason, I'm opening this in draft mode, as the respective reviews should happen independently and this one is currently a bit too cluttered for this.

@babsey: can you maybe also quickly give this a try in the context of NEST Desktop? Thanks!

@jougs jougs added S: High Should be handled next T: Maintenance Work to keep up the quality of the code and documentation. I: Behavior changes Introduces changes that produce different results for some users labels Feb 17, 2021
@jougs jougs added this to the NEST 3.0 milestone Feb 17, 2021
@jougs jougs self-assigned this Feb 17, 2021
@babsey
Copy link
Contributor

babsey commented Feb 17, 2021

can you maybe also quickly give this a try in the context of NEST Desktop? Thanks!

It works with curl and in NEST Desktop. That is great. Thanks!
In both cases, I am able to fetch documentation text via nest-server:
localhost:5000/api/help?return_text=true&obj=iaf_psc_alpha.

However, I have to execute command make html before make install.
I figured out that several models have no help text: voltmeter, static_synapse.

Is it on purpose?

@jougs
Copy link
Contributor Author

jougs commented Feb 17, 2021

@babsey: Thanks for testing so quickly and for your feedback :-)

We might have to point out the need for make html more visibly (but things are in flux, see also #1905). We could probably just return a corresponding error message instead of the actual help text from help() in case no help text is found.

Regarding the missing help files:

  • internally, the voltmeter is now just a pre-configured version of a multimeter and therefore does not have its own documentation (anymore). We should probably create a minimal help text for it that points users in the right direction.
  • the problem with synapse documentation is more fundamental: our doc extractor creates the .rst file with the same base name as the C++ header file, which is in turn based on the name of the class inside of it. This is unfortunate, because synapse model classes are called *Connection, but get registered as *_synapse. Long story short: Currently, you will get the help the help using nest.help('static_connection'), but that should definitely change.

@terhorstd: What's your opinion on the last point here? Rename all files (and live with file names that are inconsistent with the contained classes' names) or add enable the doc extractor to process a filename hint in the BeginUserDocs block?

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.

@jougs Thanks! I will wait with a full review until #1843 is merged, since that should reduce the delta quite a bit. I added one comment below.

And then a general question: Most users will be online most of the time. Why not open the helpdesk at readthedocs? The one downside is that open_new() does not tell you if the browser was able to open the webpage, one would have to check that in some other way.

pynest/nest/lib/hl_api_info.py Show resolved Hide resolved
@jougs
Copy link
Contributor Author

jougs commented Feb 18, 2021

@clinssen created #1930 to describe the naming issue more concisely and suggest a solution also for the problem described in the second bullet point above.

@jougs
Copy link
Contributor Author

jougs commented Feb 18, 2021

Replying to @heplesser:

And then a general question: Most users will be online most of the time. Why not open the helpdesk at readthedocs? The one downside is that open_new() does not tell you if the browser was able to open the webpage, one would have to check that in some other way.

I would think that opening the browser worked reliably enough (at least if we do it right, i.e. using the file:// prefix you pointed out). It's part of the standard library after all. The real downside I see with opening the version on the web is that that would get users the help for some release version, while the local one is guaranteed to be for the version they are actually running. So I would rather not change this.

Replying to @babsey:

However, I have to execute command make html before make install. I figured out that several models have no help text: voltmeter, static_synapse. Is it on purpose?

I've added yet another suggestion to #1905, which would remove the requirement for calling make userdoc (aka html) and still leave enough flexibility for developers. Any input on that would be welcome!

@heplesser
Copy link
Contributor

I would think that opening the browser worked reliably enough (at least if we do it right, i.e. using the file:// prefix you pointed out). It's part of the standard library after all. The real downside I see with opening the version on the web is that that would get users the help for some release version, while the local one is guaranteed to be for the version they are actually running. So I would rather not change this.

Good point, let's leave it as it is.

@jessica-mitchell
Copy link
Contributor

@jougs I just took at look at the help and it seems to work well for me too. Thanks for all your work on that.

I know there is still ongoing discussion about how cmake will be organized, but I agree with what you said above regarding a useful error message, if a person doesnt make html before make install. Also, to make sure that the sequence of steps is clearly visible on the CLI before you start with make

On a slightly different note, when one prints nest.help(), you get a basic list of a few commands, but I was wondering if it would be helpful to add at least a specific example for getting the model info: nest.help('model_name') and the api info: help(nest.Create) because they have two different syntaxes. And I assume those are probably the pieces of information most users want.

@jougs
Copy link
Contributor Author

jougs commented Mar 1, 2021

@jessica-mitchell: I've implemented your suggestion in 97dc9c3. Thanks!

@jougs
Copy link
Contributor Author

jougs commented Mar 1, 2021

Since #1843 was merged, this PR is based on the branch behind #1955. I'll undraft when that is merged.

@jougs
Copy link
Contributor Author

jougs commented Mar 5, 2021

Grrr! Yes, #1955 broke it. df7a476 should fix it.

@heplesser
Copy link
Contributor

@jougs Works nicely for me now :). I sent you a PR with some touch-ups for this one, hope you'll like it.

jougs and others added 2 commits March 5, 2021 23:53
Copy link
Contributor

@jessica-mitchell jessica-mitchell left a comment

Choose a reason for hiding this comment

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

@jougs Thanks, it's working nicely. I like the output on the nest.help(). My only concern is that it's not obvious you need to make html to get help working on the CLI (unless you didn't run it, and the error pops up) but maybe we should add something to the installation instructions and the output after cmake

@jessica-mitchell
Copy link
Contributor

Also @jougs, @stinebuu pointed out in #1961 that there is a broken image link (right before the first note) in the user-level documentation workflow page. Maybe that can be addressed here since it should be an easy fix?

@jougs
Copy link
Contributor Author

jougs commented Mar 16, 2021

@jessica-mitchell: the broken link is fixed by commit d7e2006. This PR thus also fixes #1961.

@jougs jougs linked an issue Mar 16, 2021 that may be closed by this pull request
@jougs
Copy link
Contributor Author

jougs commented Mar 16, 2021

@lekshmideepu: sorry to bother you with this, but could you please have a look at the failing GitHub Action jobs? It seems that some of the sources for apt are not working anymore. #1899 has more failing builds, in case you need more samples... Thanks!

@sarakonradi
Copy link
Contributor

Just a brief update: @jessica-mitchell and I agreed to address the image links in a separate PR so that the graphics can work sooner rather than later. #1979 fixes them.

Copy link
Contributor

@jessica-mitchell jessica-mitchell left a comment

Choose a reason for hiding this comment

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

I have no further changes, thanks

@heplesser heplesser merged commit 34c223a into nest:master Mar 18, 2021
@jougs jougs deleted the fix-pynest-help branch August 31, 2023 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: Behavior changes Introduces changes that produce different results for some users S: High Should be handled next T: Maintenance Work to keep up the quality of the code and documentation.
Projects
Status: Done
5 participants