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 api generation for documentation using mock #1274

Merged
merged 18 commits into from
Nov 27, 2019

Conversation

jessica-mitchell
Copy link
Contributor

This PR provides a solution for the PyNEST api generation on readthedocs.

The problem: pynest apis do not work on readthedocs.
For sphinx to auto generate the apis (sphinx-autodoc) for the website, it requires a running instance of nest. If we try to include nest in the build on ReadtheDocs, it times out because of resource limitations.

The solution:
Mock nest
All the credit goes to @Silmathoron, who provided the scripts/code required to mock nest!

The PyNEST apis functions were cleaned up a bit then indexed, and referenced in the docs

See output here: https://nest-test.readthedocs.io/en/pynest_mock/ref_material/pynest_apis.html

jessica-mitchell and others added 3 commits August 27, 2019 10:02
remove some content in connect function, provide link
to connection managment guide, add func link and minor formatting changes

removed old connecton info, update syntax and fix formatting issues

remove whitespace, rtd req, add topology module

add py and cpp to ignore and update headings
@terhorstd
Copy link
Contributor

we decided to put this on hold for two weeks, because other PRs handle similar Python issues...

@terhorstd terhorstd added ZC: Documentation DO NOT USE THIS LABEL P: Blocked Work on this can not continue, see comments for the particular reason S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Sep 2, 2019
@Silmathoron
Copy link
Member

@terhorstd sorry, this is a little unclear to me: which PRs are these? Which Python issues?

@jessica-mitchell
Copy link
Contributor Author

@Silmathoron The issue he's referring to is the failing check on Travis with Python 2.7

@Silmathoron
Copy link
Member

I just checked and it seems that the errors should be easy to fixed: mostly copyright header errors and maybe some other formatting errors that should be fixed automatically by using extras/check_copyright_headers.py and pep8fying

MSGBLD0220: Python files with formatting errors:
	MSGBLD0220: ... doc/conf.py
	MSGBLD0220: ... doc/pynestkernel_mock.py
	MSGBLD0220: ... pynest/nest/lib/hl_api_connections.py
	MSGBLD0220: ... pynest/nest/lib/hl_api_exceptions.py

Co-Authored-By: Tanguy Fardet <Silmathoron@users.noreply.github.com>
@Silmathoron
Copy link
Member

Wow,
apparently the metaclass argument in Py3 is not only the metaclass but also inheritance I guess...
Is the metaclass really necessary? Can you check whether simple inheritance is sufficient?
@jessica-mitchell

@jessica-mitchell
Copy link
Contributor Author

@Silmathoron I'm afraid my Python-fu is fairly limited - and looking at the code I am not really confident with what I would all need to change to test if simple inheritance would work.

@Silmathoron
Copy link
Member

OK, I'll try to check it at some point in the week-end or next week.

@jessica-mitchell jessica-mitchell added ZP: PR Created DO NOT USE THIS LABEL and removed P: Blocked Work on this can not continue, see comments for the particular reason labels Nov 24, 2019
@jessica-mitchell
Copy link
Contributor Author

@Silmathoron During our 'docathon' thanks to @hakonsbm, we were able to solve the issues with Travis - could you take a look and approve this PR if there is nothing else we need?

Copy link
Member

@Silmathoron Silmathoron left a comment

Choose a reason for hiding this comment

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

Nice work! Sorry I did not have time to look into it before in the end.
I'm wondering about the choice of always using the mocked kernel.
Otherwise, if possible I would generalize the use of references using the :func:`function_name nomenclature in the .rst files

doc/conf.py Outdated
@@ -31,20 +31,55 @@
import sys
import os

# import sphinx_gallery
Copy link
Member

Choose a reason for hiding this comment

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

remove this line


# -- Mock pynestkernel ----------------------------------------------------
# The mock_kernel has to be imported after setting the correct sys paths.
from mock_kernel import convert # noqa
Copy link
Member

Choose a reason for hiding this comment

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

why not use the proper kernel if the install is not done on RtD?
If this really is a feature, then remove following comment lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were thinking for consistency, just leaving the mock_kernel for non RtD builds too; it then allows the docs to build without a proper nest install - Not sure if that's a 'feature', but could be useful.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I don't have any preference, but if we keep the mock_kernel for all builds, then comments should go away ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do :)

doc/conf.py Outdated
@@ -218,8 +265,6 @@ def setup(app):
]

# -- Options for readthedocs ----------------------------------------------
# on_rtd = os.environ.get('READTHEDOCS') == 'True'
# if on_rtd:
# html_theme = 'alabaster'
Copy link
Member

Choose a reason for hiding this comment

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

also remove these lines

`SLI <an-introduction-to-sli.md>`__ ,the old syntax of the function
still works, while in `PyNEST <introduction-to-pynest.md>`__, the
``Connect()`` function has been renamed to ``OneToOneConnect()``.
`Connect()` function has been renamed to `OneToOneConnect()`.
Copy link
Member

Choose a reason for hiding this comment

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

for existing functions, it would be nice to use the proper form to link directly to the corresponding docstring
either through :func:`nest.Connect or :func:`nest.lib.hl_api_connections.Connect depending on whether or not the full path is necessary.
do this to all functions in the API if possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For creating these links, I'd like to wait for another PR -- we have a script that can search through all function occurrences and replace with the proper format.


* The `Command Index <https://www.nest-simulator.org/helpindex/>`_ contains a list of all SLI and C++ related reference material.

Copy link
Member

Choose a reason for hiding this comment

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

remove extra blank lines

@@ -522,7 +475,7 @@ def CGParse(xml_filename):
generator cg.

The library to provide the parsing can be selected
by CGSelectImplementation().
by :py:func:`.CGSelectImplementation`.
Copy link
Member

Choose a reason for hiding this comment

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

OK, if this link works, use this format for all links in previous .rst files (not sure the initial :py is actually necessary, is it?)

Copy link
Member

@Silmathoron Silmathoron left a comment

Choose a reason for hiding this comment

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

OK, I just compiled the docs on my computer without issues, except for the fact that sphinx_gallery and sphinx_tabs are necessary and crash the build if not there... we should either mention it somewhere or make sure that a partial docs build is still done even if they are not there.

Apart from that, I would think we should make the PyNEST API more easy to find (Reference material > PyNEST functions) does not quite cut it for me (it is usually one of the first thing I am looking for in a documentaion).

Finally, I the API, I would put the "helper functions" at the very end and add a

.. contents::
   :local:

at the beginning

@jessica-mitchell
Copy link
Contributor Author

@Silmathoron Thanks for your input! I updated the pynest apis so they appear on the side bar, they have an internal toc and are rearranged.

As for the sphinx_gallery and tabs - these should be listed in the requirements.txt file in doc/. (We still need to update this and the doc README) in a separate PR.

Copy link
Member

@Silmathoron Silmathoron left a comment

Choose a reason for hiding this comment

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

Nice! thanks for the hard work 👍

Copy link
Contributor

@steffengraber steffengraber left a comment

Choose a reason for hiding this comment

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

Very nice work. I tested it and it works great. Just a few small comments.

Comment on lines 22 to 24
'''
Mock pynestkernel.pyx into dummy python file.
'''
Copy link
Contributor

Choose a reason for hiding this comment

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

Tripel double-quoted strings should be used for docstrings and a title is missing.



def convert(infile):
'''
Copy link
Contributor

Choose a reason for hiding this comment

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

See my comment above.

@hakonsbm hakonsbm merged commit 5a79531 into nest:master Nov 27, 2019
@jessica-mitchell jessica-mitchell deleted the pynest_mock branch March 30, 2021 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. ZC: Documentation DO NOT USE THIS LABEL ZP: PR Created DO NOT USE THIS LABEL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants