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

Test with sphinx 6 #1135

Closed
wants to merge 4 commits into from
Closed

Test with sphinx 6 #1135

wants to merge 4 commits into from

Conversation

drammock
Copy link
Collaborator

This PR hopefully gets us a CI environment that runs Sphinx 6. It does this by:

  • in pyproject.toml, separates out mystnb and sphinx-design because they put an upper pin on sphinx. This allows us to do pip install -e .[test] and get an environment that includes sphinx 6 instead of sphinx 5. I cherry-picked a commit from @nicoa from ENH: split doc generation to avoid sphinx-design requiring nonnewest … #1113 to give credit; but this will supersede that PR.
  • adds a new workflow file that only runs tests (not lint, audit, doc build, etc) and runs them only on 2 configs: oldest python + oldest sphinx, and newest python + newest sphinx. Currently this is Sphinx 4.2 on Python 3.7, and Sphinx 6.x on Python 3.12 alpha.
  • cleans up the existing .github/workflows/tests.yml to remove redundancy (i.e., don't need py 3.7 in the matrix anymore) and to standardize the other (non-matrix) jobs to all use the same Python version.

closes #1113

"sphinx-togglebutton",
# Install nbsphinx in case we want to test it locally even though we can't load
# it at the same time as MyST-NB.
"nbsphinx",
"ipyleaflet",
]
doc = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

we are now missing rich in the documentation build on RDT

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep I saw that but I don't know why. [doc] inherits from [doc_base] so rich ought to be installed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

when the pyproject.toml file specifies pydata-sphinx-theme[doc_base] is it equivalent to .[doc-base] or is it looking to the latest available on pipy ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ahh, good point. I think @nicoa made this point in another issue and I still forgot 🤦🏻

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry that was an open question, I actually don't know what is the syntaxe to use to nest extra_require

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried with .[whatever] and it fails. If I understand pypa/pip#10393 (comment) correctly then it actually should work (with pip >= 21.2) to use pydata-sphinx-theme[whatever] and it will look within the local pyproject.toml file (instead of looking at only released versions on PyPI), so I don't know why it's not working like that here.

Copy link
Contributor

@nicoa nicoa Jan 25, 2023

Choose a reason for hiding this comment

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

yes, addressed this somewhere (that we first need a (pre-)release) - thanks for pointing to the pip issue, that is a good hint. Unfortunate that it didn't work out still :-(

cross-link: #1113 (comment)

@drammock
Copy link
Collaborator Author

In the end, I don't think this approach is very good. I'm going to close and re-think our strategy here. Also (note to self) we already have .github/workflows/prerelease.yml that runs twice weekly, but it is going to be hamstrung by the Sphinx pin too, so it's not really doing its job at the moment.

cc @jarrodmillman, do you have any bright ideas about how to solve this? If you haven't been following along, the TL;DR is that two of our doc-build dependencies (sphinx-design and mystnb) place upper pins on sphinx so we're always ending up with Sphinx 5.3 in the tests, never testing against sphinx 6.x. Here I tried separating out those two dependencies into their own project.optional-dependencies group so that they wouldn't get installed via pip install -e .[test] --- but that fails at the installation stage because the new grouping isn't available in any released version of our theme yet.

@drammock drammock closed this Jan 24, 2023
@12rambau
Copy link
Collaborator

just my 2 cents here:
In my libs I never nest extra_require with one another. Instead I try as much as I can to to split things (even if it lead to small overlays), which can easily be done as we are using nox to build tests and doc build. If I really need 2 of them I specify the 2 in the install:

foo[doc,test]

@choldgraf
Copy link
Collaborator

Why don't we make our test suite only depend on minimal dependencies, and for all of these extensions we have CSS support for, we can use our docs to see how they look.

@drammock
Copy link
Collaborator Author

Why don't we make our test suite only depend on minimal dependencies, and for all of these extensions we have CSS support for, we can use our docs to see how they look.

That's an option. I tried the following:

mamba create -n pst6 "sphinx>=6" beautifulsoup4 "docutils!=0.17.0" packaging "pygments>=2.7" accessible-pygments pytest pytest-regressions nox
mamba activate pst6
pip install -e .  # all reqs already satisfied
nox -s test --no-venv

and only two tests fail (the last two in the file, test_ablog and test_empty_template, which both involve ablog). @choldgraf since you seem to be the main ablog support advocate, WDYT we should do about those two tests?

@drammock
Copy link
Collaborator Author

I'm wary of this part:

for all of these extensions we have CSS support for, we can use our docs to see how they look

because in my experience, relying on people to look at built docs for QA on (almost) every PR is unreliable --- partly because people are busy/lazy, and partly because unforeseen side effects of a change might show up on a page that is not expected to have been affected, so even if a conscientious reviewer does look at the docs, they might not look carefully enough at all the pages such that they would notice/catch that unforeseen side effect.

That said, it's basically what we're already doing 😅 so it's not like a big step backwards... it just means we are deciding that in future we won't add any new tests that use our optional dependencies. In other words, we're committing to relying on (1) a successful Sphinx build to indicate everything is mostly OK, and (2) users telling us when something looks weird in our docs (or their sites).

@choldgraf
Copy link
Collaborator

yeah I think the main thing is that we aren't actually testing for functionality of those extensions in our test suite anyway, we still mostly just use the eye-test and the github issues test to know when there are issues with a particular library (especially since "supporting a library" generally just means having dedicated CSS for it).

@drammock drammock deleted the test-sphinx6 branch March 21, 2023 19:55
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.

4 participants