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

Usage with RTD - sphinx>=2 requirement was optional? #139

Closed
consideRatio opened this issue May 9, 2020 · 7 comments · Fixed by #170
Closed

Usage with RTD - sphinx>=2 requirement was optional? #139

consideRatio opened this issue May 9, 2020 · 7 comments · Fixed by #170
Labels
enhancement New feature or request

Comments

@consideRatio
Copy link

Read the docs is a bit messy to work with sometimes, because it comes with sphinx<2 pre-installed, so when we run pip install -r doc-requirements.txt it will settle for version 1 unless a dependency forces is to go higher.

But, it seems MyST-Parser have a hard dependency of sphinx>1 due to the following error:

  File "/home/docs/checkouts/readthedocs.org/user_builds/zero-to-jupyterhub/envs/latest/lib/python3.7/site-packages/myst_parser/sphinx_renderer.py", line 16, in <module>
    from sphinx.project import Project

Due to this, I suggest to add the requirement of sphinx >=2 directly in MyST-Parser.

Z2JH setup that broke

doc-requirements.txt

pydata_sphinx_theme
pyyaml
myst_parser
sphinx-autobuild
sphinx-copybutton
sphinx<3

.readthedocs.yml

# Configuration on how ReadTheDocs (RTD) builds our documentation
# ref: https://readthedocs.org/projects/zero-to-jupyterhub/
# ref: https://docs.readthedocs.io/en/stable/config-file/v2.html

# Required (RTD configuration version)
version: 2

# Build documentation in the docs/ directory with Sphinx
sphinx:
  configuration: doc/source/conf.py

# Optionally build your docs in additional formats such as PDF and ePub
formats: all

# Optionally set the version of Python and requirements required to build your docs
python:
  version: 3.7
  install:
    # WARNING: This requirements file will be installed without the pip
    #          --upgrade flag in an existing environment. This means that if a
    #          package is specified without a lower boundary, we may end up
    #          accepting the existing version.
    #
    #          ref: https://github.com/readthedocs/readthedocs.org/blob/0e3df509e7810e46603be47d268273c596e68455/readthedocs/doc_builder/python_environments.py#L335-L344
    - requirements: doc/doc-requirements.txt
@consideRatio consideRatio added the bug Something isn't working label May 9, 2020
@consideRatio
Copy link
Author

consideRatio commented May 9, 2020

Hmmm...

MyST-Parser/setup.py

Lines 40 to 52 in f32bd38

install_requires=["markdown-it-py~=0.4.5"],
extras_require={
"sphinx": ["pyyaml", "docutils>=0.15", "sphinx>=2,<3"],
"code_style": ["flake8<3.8.0,>=3.7.0", "black", "pre-commit==1.17.0"],
"testing": [
"coverage",
"pytest>=3.6,<4",
"pytest-cov",
"pytest-regressions",
"beautifulsoup4",
],
"rtd": ["sphinxcontrib-bibtex", "ipython", "sphinx-book-theme"],
},

Ah, so MyST-Parser does not require sphinx directly unless specifying it like myst_parser[sphinx]? Then I wonder what makes sense for Z2JH to declare. There are two sets that could be relevant, both sphinx and rtd.

Suggestions on what to do here? For now, I'll go to Z2JH and make it explicit that we want sphinx>=2,<3 and then I think the issue I ran into may go away, but perhaps there is a change that makes sense for MyST-Parser still given this experience. I'm not sure...

@consideRatio consideRatio changed the title Dependency of sphinx>=2 isn't explicit Usage with RTD - sphinx>=2 requirement was optional? May 9, 2020
@chrisjsewell
Copy link
Member

Hey @consideRatio, thanks for the feedback. As mentioned in the documentation (https://myst-parser.readthedocs.io/en/latest/using/intro.html#installation) you should use the sphinx extra. The rtd is only really for internal use, to build the docs for this repo.
You know you can use myst-parser[sphinx] in your requirements.txt?

@consideRatio
Copy link
Author

@chrisjsewell excellent okay I understand, I'll go for myst-parser[sphinx] in my requirements.txt file.

And here I'm confirming what you say makes sense:

version: 2
python:
version: 3
install:
- method: pip
path: .
extra_requirements:
- sphinx
- rtd


I'm considering if there is a concrete action point following this. I found the documentation to be good already, and my issue was mostly that I was outdated with the documentation.

But perhaps having rtd in the setup.py file's extras_require is a bit confusing to others as well? It felt to me as it could been a hard requirements for using myst-parser on RTD in general, but it ended up being specifically what this repo needed to build its own opinionated docs.

I think what makes sense is to add an inline comment saying that the rtd part is what is required for this repo specifically, or extract both rtd and testing requirements out of the setup.py file, but that is probably adding complexity without much benefit.

@chrisjsewell
Copy link
Member

Ok no problem, I can add a comment in the setup.py 👍

@consideRatio
Copy link
Author

consideRatio commented May 9, 2020

@choldgraf and @chrisjsewell for your information, watch out for this.

# with sphinx==1.* already installed as on RTD, this results in sphinx==1.*
pydata-sphinx-theme # depends on sphinx
myst-parser[sphinx] # depends on sphinx<3,>=2
# with sphinx==1.* already installed as on RTD, this results in sphinx==2.*
myst-parser[sphinx] # depends on sphinx<3,>=2
pydata-sphinx-theme # depends on sphinx

@chrisjsewell chrisjsewell added enhancement New feature or request and removed bug Something isn't working labels May 9, 2020
@chrisjsewell chrisjsewell added this to To do in Stable Release via automation May 9, 2020
@mlncn
Copy link

mlncn commented Jun 2, 2020

I was about to open a separate issue for documenting the simplest way to use MyST on ReadTheDocs— i probably still should. 'Cause yeah i saw this documentation itself is hosted on readthedocs but i couldn't follow how, so knew there had to be simpler setups. So thrilled to know about this project, thanks Chris!

@choldgraf
Copy link
Member

good call @mlncn - I opened up #155 to track it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging a pull request may close this issue.

4 participants