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

cylc domain #13

Merged
merged 16 commits into from
Mar 18, 2020
Merged

cylc domain #13

merged 16 commits into from
Mar 18, 2020

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Nov 15, 2019

Closes #11
Partly addresses #12

It's hard-to-review code-bomb time! Sorry! I'll try to comment this better to help.

The code is documented in module docstrings, you can build it with:

$ make clean html
$ firefox _build/html/index.html

(note: the docs use a strict build so building the docs also tests them)

Cylc Domain

Create a Sphinx domain for documenting Cylc configurations. This allows us to document settings as objects which we can reference throughout the Cylc documentation (and via inter-sphinx any Sphinx project).

Benefits:

  • A formal way to document configurations.
  • A formal way to reference configurations internally and externally.
  • References get checked on build so if we reference a configuration that has been removed or renamed we will get an error.

Definition examples:

Cylc configurations can be documented manually like this:

.. cylc:conf:: myconf

   Description of myconf

   .. cylc:section:: foo

      Description of foo

      .. cylc:setting:: bar

        :type: string
        :default: pub

        Description of bar.

Reference examples:

Once defined objects can be referenced like this:

  • :cylc:conf:'suite.rc'
  • :cylc:section:'[runtime]'
  • :cylc:setting:'[scheduling]max active cycle points'

Cylc AutoDocumenter

This PR also adds a POC autodocumenter which can document Cylc configurations from SPEC dictionaries (at present only type, default and options are available). This will need to be re-worked when we build documentation into Cylc configurations.

Parsec examples:

Parsec types can be be autodocumented like so (requires cylc/cylc-flow#3439):

.. auto-cylc-type::
   cylc.flow.parsec.validate.ParsecValidator.V_TYPE_HELP
   cylc.flow.parsec.validate.CylcConfigValidator.V_TYPE_HELP

Cylc examples:

Cylc configs can be autodocumented like so:

.. auto-cylc-conf:: suite cylc.flow.cfgspec.suite.SPEC

@oliver-sanders oliver-sanders added this to the 1.1.0 milestone Nov 15, 2019
@oliver-sanders oliver-sanders self-assigned this Nov 15, 2019
@oliver-sanders oliver-sanders force-pushed the cylc-domain branch 2 times, most recently from 8ce950b to c394af1 Compare February 17, 2020 23:13
@oliver-sanders
Copy link
Member Author

oliver-sanders commented Feb 17, 2020

The Travis build is failing due to an interaction between hieroglyph + docutils==0.16.0

For now I've pinned docutils to <0.16.0 until the cause/workaround is determined. Booted to #14

@oliver-sanders oliver-sanders force-pushed the cylc-domain branch 2 times, most recently from 5143918 to 374b469 Compare February 17, 2020 23:47
@oliver-sanders oliver-sanders mentioned this pull request Feb 17, 2020
@oliver-sanders
Copy link
Member Author

NOTE:

This can go in before cylc/cylc-flow#3253 is attempted, the autodocumenter will need to be updated to work with whatever replaces the SPEC dictionary.

@oliver-sanders oliver-sanders marked this pull request as ready for review February 18, 2020 00:10
Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Some typos, but I don't think we need to fix all of them? This is a module used mainly by ourselves, so feel free to resolve any typo if that doesn't result in a typo in the generated doc 👍 (I am not sure whether they do or not).

Tried to simply build it with:

  1. Check out branch
  2. Create new Py3 virtual env
  3. pip install -e .[all]
  4. Repeat above in ../cylc-flow to get latest Cylc Flow module
  5. make html singlehtml

Then it failed with the following error:

Warning, treated as error:
/home/kinow/Development/python/workspace/cylc-sphinx-extensions/venv/lib/python3.7/site-packages/sphinx/ext/autosummary/templates/autosummary/base.rst:3:Error in "currentmodule" directive:
maximum 1 argument(s) allowed, 3 supplied.

.. currentmodule:: {{ module }}
Makefile:23: recipe for target 'html' failed
make: *** [html] Error 2

@oliver-sanders is that expected until cylc/cylc-flow#3253 is done? If so will approve this PR.

cylc/sphinx_ext/diff_selection/__init__.py Outdated Show resolved Hide resolved
cylc/sphinx_ext/grid_table/__init__.py Outdated Show resolved Hide resolved
cylc/sphinx_ext/hieroglyph_addons/__init__.py Outdated Show resolved Hide resolved
cylc/sphinx_ext/hieroglyph_patch/__init__.py Outdated Show resolved Hide resolved
cylc/sphinx_ext/rtd_theme_addons/__init__.py Outdated Show resolved Hide resolved
cylc/sphinx_ext/cylc_lang/__init__.py Outdated Show resolved Hide resolved
def process_link(self, env, refnode, has_explicit_title, title, target):
# copy ref_context to the refnode so that we can access it in
# resolve_xref. Note that walking through the node tree to extract
# ref_context items appears only to work in the HTML buider.
Copy link
Member

Choose a reason for hiding this comment

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

s/buider/builder

@oliver-sanders
Copy link
Member Author

@oliver-sanders is that expected until cylc/cylc-flow#3253 is done? If so will approve this PR.

Hmmm, curious, singlehtml works for me, will try and re-produce.

@kinow
Copy link
Member

kinow commented Feb 18, 2020

@oliver-sanders is that expected until cylc/cylc-flow#3253 is done? If so will approve this PR.

Hmmm, curious, singlehtml works for me, will try and re-produce.

I think the html target failed first. But from your previous comment it looks like it worked for you too. I wonder if I am missing something else?

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Feb 18, 2020

I can't replicate this ATM, here's my attempt to reproduce:

$ conda create -y -n cse
$ conda activate cse
$ conda install -y -c conda-forge python==3.7
$ cd ~/cylc-flow/; pip install -e .[all]
$ cd ~/cylc-sphinx-extensions; pip install -e .[all]
$ conda install graphviz  # one thing we cant package with pip
$ make clean html

Is your virtualenv pulling in Sphinx or Docutils from the base installation?

import sphinx
print(
    sphinx.__version__
    sphinx.__file__
)
import docutils
print(
    docutills.__version__,
    docutills.__file__
)

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Feb 18, 2020

Ooh, actually try this:

diff --git a/index.rst b/index.rst
index 5b53b99..1bb0b5a 100644
--- a/index.rst
+++ b/index.rst
@@ -5,7 +5,6 @@ Available Extensions
 
 .. autosummary::
    :toctree: extensions
-   :template: mymodule.rst
 
    cylc.sphinx_ext.cylc_lang
    cylc.sphinx_ext.diff_selection

@kinow
Copy link
Member

kinow commented Feb 18, 2020

Ooh, actually try this

$ rm _templates/mymodule.rst
$ make clean html

I was installing graphviz when your comment arrived. I had the same error that I had with pip. Then tried your commands above (after conda install graphviz). Now got:

(cse) kinow@kinow-VirtualBox:~/Development/python/workspace/cylc-sphinx-extensions$ rm _templates/mymodule.rst
(cse) kinow@kinow-VirtualBox:~/Development/python/workspace/cylc-sphinx-extensions$ make clean html
rm -r extensions/ || true
Removing everything under '_build'...
# build documentation
Running Sphinx v2.4.1
making output directory... done
[autosummary] generating autosummary for: README.rst, index.rst, venv/lib/python3.7/site-packages/Jinja2-2.11.1.dist-info/LICENSE.rst, venv/lib/python3.7/site-packages/Rx-1.6.1.dist-info/DESCRIPTION.rst, venv/lib/python3.7/site-packages/alabaster-0.7.12.dist-info/DESCRIPTION.rst, venv/lib/python3.7/site-packages/ansimarkup-1.4.0.dist-info/DESCRIPTION.rst, venv/lib/python3.7/site-packages/certifi-2019.11.28.dist-info/DESCRIPTION.rst, venv/lib/python3.7/site-packages/chardet-3.0.4.dist-info/DESCRIPTION.rst, venv/lib/python3.7/site-packages/codecov-2.0.15.dist-info/DESCRIPTION.rst, venv/lib/python3.7/site-packages/hieroglyph-1.0.0.dist-info/DESCRIPTION.rst, ..., venv/lib/python3.7/site-packages/imagesize-1.2.0.dist-info/LICENSE.rst, venv/lib/python3.7/site-packages/importlib_metadata/docs/changelog.rst, venv/lib/python3.7/site-packages/importlib_metadata/docs/index.rst, venv/lib/python3.7/site-packages/importlib_metadata/docs/using.rst, venv/lib/python3.7/site-packages/pycodestyle-2.5.0.dist-info/DESCRIPTION.rst, venv/lib/python3.7/site-packages/pytest_cov-2.8.1.dist-info/AUTHORS.rst, venv/lib/python3.7/site-packages/pytz-2019.3.dist-info/DESCRIPTION.rst, venv/lib/python3.7/site-packages/sphinx/ext/autosummary/templates/autosummary/base.rst, venv/lib/python3.7/site-packages/sphinx/ext/autosummary/templates/autosummary/class.rst, venv/lib/python3.7/site-packages/sphinx/ext/autosummary/templates/autosummary/module.rst

Exception occurred:
  File "/home/kinow/anaconda3/envs/cse/lib/python3.7/site-packages/sphinx/jinja2glue.py", line 213, in get_source
    raise TemplateNotFound(template)
jinja2.exceptions.TemplateNotFound: mymodule.rst
The full traceback has been saved in /tmp/sphinx-err-ub5fjd5s.log, if you want to report the issue to the developers.
Please also report this if it was a user error, so that a better error message can be provided next time.
A bug report can be filed in the tracker at <https://github.com/sphinx-doc/sphinx/issues>. Thanks!
Makefile:23: recipe for target 'html' failed
make: *** [html] Error 2

@kinow
Copy link
Member

kinow commented Feb 18, 2020

Checked out the _templates/mymodule.rst file again, same (old) error.

reading sources... [100%] venv/lib/python3.7/site-packages/sphinx/ext/autosummary/templates/autosummary/module                                                                                                     

Warning, treated as error:
/home/kinow/Development/python/workspace/cylc-sphinx-extensions/venv/lib/python3.7/site-packages/importlib_metadata/docs/index.rst:24:toctree contains reference to nonexisting document 'venv/lib/python3.7/site-packages/importlib_metadata/docs/changelog (links)'
Makefile:23: recipe for target 'html' failed
make: *** [html] Error 2

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Feb 18, 2020

(sorry, you need to apply the diff rather than remove the file, corrected comment above)

@kinow
Copy link
Member

kinow commented Feb 18, 2020

(sorry, you need to apply the diff rather than remove the file, corrected comment above)

Diff applied (first time I had the error there were no new files, but I later I tried git clean -d -f to make sure no new files were causing this error).

image

Still same error (tried with deactivate; conda activate cse, and later conda deactivate; source venv/bin/activate, both Conda & pip failing).

Is your virtualenv pulling in Sphinx or Docutils from the base installation?

venv

image

conda

image

It could be something else in my environment. Maybe another reviewer could try it later and see if that works. I can also try from home later today to check if I get the same error or not?

@oliver-sanders
Copy link
Member Author

Those versions match what I'm testing with and the paths look file.

I have no idea what's going wrong for you. The tests are passing on Travis, for info all Travis does to install is this:

    - pip install git+https://github.com/cylc/cylc-flow/  # install latest cylc-flow
    - pip install -e .[all]
    - npm install -g eslint
    - sudo apt-get install -y graphviz

The only thing left is to examine the full traceback stack, you'll find this in the sphinx output:

The full traceback has been saved in /tmp/sphinx-err-ub5fjd5s.log

Perhaps post to Riot as it'll be quite long.

@kinow
Copy link
Member

kinow commented Feb 18, 2020

The only thing left is to examine the full traceback stack, you'll find this in the sphinx output:

The full traceback has been saved in /tmp/sphinx-err-ub5fjd5s.log

Perhaps post to Riot as it'll be quite long.

That traceback was from when I did a rm _templates/mymodule.rst.

Those versions match what I'm testing with and the paths look file.

I have no idea what's going wrong for you. The tests are passing on Travis, for info all Travis does to install is this:

    - pip install git+https://github.com/cylc/cylc-flow/  # install latest cylc-flow
    - pip install -e .[all]
    - npm install -g eslint
    - sudo apt-get install -y graphviz

It must be something I'm doing wrong. Just tried at home, and got the same error. Will try to copy the Travis installation steps and see if that works for me. Thanks!

@kinow
Copy link
Member

kinow commented Feb 18, 2020

@oliver-sanders found it! And the cause was my venv folder. Even with the Conda environment, that venv directory was still being used — apparently.

I decided to search for that error, and got few uninteresting results. And one in Japanese (normally I see those in Japanese or Chinese and ignore). But this one was spot on, even the solution/explanation was good.

Translated: https://translate.google.com/translate?sl=ja&tl=en&u=http%3A%2F%2Fcointoss.hatenablog.com%2Fentry%2F2013%2F03%2F31%2Fsphinx-including-virtualenv

Cause

The cause was that the rst file included in sphinx installed under .env was also targeted for build.

If sphinx does not separate the source directory, the current directory is specified as the source directory * 1
In that case, .env is also specified as the source folder, and an error occurs when trying to build a .rst file that exists deep in .env.

And the solution worked for me:

solution

Exclude .env from build targets.

Add .env to exclude_patterns in conf.py. Since it is added as a character string, enclose it in ''.

But I had to include the following:

diff --git a/conf.py b/conf.py
index 9d04f02..662df75 100644
--- a/conf.py
+++ b/conf.py
@@ -59,3 +59,8 @@ autosummary_imported_members = True
 intersphinx_mapping = {
     'sphinx': ('http://www.sphinx-doc.org/', None)
 }
+
+# List of patterns, relative to source directory, that match files and
+# directories to ignore when looking for source files.
+exclude_patterns = ['_build', 'venv']
+

With conda it failed because I had not deleted my venv folder.

Approving the PR. But as others could use pip, maybe we should add this exclude patterns just to play safe?

Thanks for the help troubleshooting it!

@oliver-sanders
Copy link
Member Author

Aah, creating a virtualenv in the root directory, nice detective work, have applied your fix.

@oliver-sanders oliver-sanders requested a review from wxtim March 15, 2020 21:46
Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

Checks

  • Read the changes
  • Checked out and tried using it (followed @kinow 's process)

Comments

  • A few spelling errors - I added suggestions to make correcting these easier.
  • Had to specify Python 3.7 in conda create - 3.8 cause a failure to pip install pyzmq - this may be an issue ahead?
  • How long should the "singlehtml" output be? It looked a bit short to me:
>  cat $PWD/_build/singlehtml/index.html | wc -l
323

@oliver-sanders
Copy link
Member Author

How long should the "singlehtml" output be? It looked a bit short to me:

Never tried singlehtml, the "stub" pages don't seem to get included, no idea, shouldn't matter though.

@kinow
Copy link
Member

kinow commented Mar 16, 2020

.travis.yml conflicts

@kinow
Copy link
Member

kinow commented Mar 18, 2020

Two approvals, and all checks passing. LGTM

@kinow kinow merged commit a8d77cd into cylc:master Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cylc domain
3 participants