Skip to content
This repository has been archived by the owner on Aug 19, 2023. It is now read-only.

Fix most Sphinx warnings #1737

Merged
merged 12 commits into from
May 22, 2023
Merged

Conversation

Eric-Arellano
Copy link
Collaborator

@Eric-Arellano Eric-Arellano commented May 17, 2023

Fixes the majority of docs failures, which are broadly these categories:

  • Invalid references
  • Invalid Sphinx syntax
  • Config out of sync with qiskit-terra, e.g. missing doctest and qasm3 requirements.

We can't yet enable sphinx-build -W because there are some bigger issues still:

@Eric-Arellano Eric-Arellano changed the title [WIP] Fix all Sphinx warnings [WIP] Fix most Sphinx warnings May 18, 2023
@Eric-Arellano Eric-Arellano marked this pull request as ready for review May 18, 2023 23:17
@Eric-Arellano Eric-Arellano changed the title [WIP] Fix most Sphinx warnings Fix most Sphinx warnings May 18, 2023
Copy link
Collaborator Author

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Ready for review. I will need an admin to remove tests-python3.7-ubuntu-latest from the CI check in Branch Protection in GitHub config, please.

.github/workflows/main.yml Outdated Show resolved Hide resolved
# Jinja2 3.1.0 is incompatible with sphinx and/or jupyter until they are updated
# to work with the new jinja version (the jinja maintainers aren't going to
# fix things) pin to the previous working version.
jinja2==3.0.3
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't pin this in Terra. Seemed unnecessary.

Copy link
Member

@jakelishman jakelishman May 22, 2023

Choose a reason for hiding this comment

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

I'm pretty sure we did right when jinja2 3.1 was released, but we subsequently released the pin on Terra and forgot to do so on the metapackage.

edit: yeah, Qiskit/qiskit#7815 added it, and Qiskit/qiskit#8968 removed it again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are all copied from qiskit-tutorials.

@@ -5,7 +5,6 @@ qiskit-sphinx-theme~=1.11.0
pylatexenc>=1.4
sphinx-automodapi
jupyter
jupyter-sphinx
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Forgot to delete this in a prior PR.

@@ -52,12 +54,12 @@
"sphinx.ext.mathjax",
"sphinx.ext.viewcode",
"sphinx.ext.extlinks",
"jupyter_sphinx",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Forgot to delete this in a prior PR.

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

Thanks for this. Would the CI pass if we ran with an installation requirement qiskit-qasm3-import; python_version>'3.7', or would it fail somewhere else instead?

If testing qiskit-qasm3-import is what would cause us to need to drop Python 3.7 from this test immediately, I ought to be able to add support to that package without too much effort, but if there's a quick way to avoid trying it, I won't bother (since the package is slated for replacement anyway).

docs/images/gallery_shot.png Outdated Show resolved Hide resolved
- Added a new class :class:`~.Z2Symmetries` to :mod:`qiskit.quantum_info`
- Added a new class :class:`~qiskit.quantum_info.Z2Symmetries` to :mod:`qiskit.quantum_info`
Copy link
Member

Choose a reason for hiding this comment

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

Not important, but just for your writing convenience if you like in the future: you can combine the ~ (strip leading prefixes) and . (suffix lookup) operators in Sphinx at whatever level of prefix you like. In :class:`~.Z2Symmetries` the ~ is redundant because there's no prefix to strip, but :class:`~.quantum_info.Z2Symmetries` would use quantum_info.Z2Symmetries as the suffix to lookup (and so be disambiguated), then ~ would cause quantum_info to be stripped in the output display.

docs/release_notes.rst Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thanks for this. Would the CI pass if we ran with an installation requirement qiskit-qasm3-import; python_version>'3.7', or would it fail somewhere else instead?

For this immediate PR, CI passes with qiskit-qasm3-import. But this is all prework for getting tox -e docs to use -W. So it would block enabling -W.

If testing qiskit-qasm3-import is what would cause us to need to drop Python 3.7 from this test immediately, I ought to be able to add support to that package without too much effort, but if there's a quick way to avoid trying it, I won't bother (since the package is slated for replacement anyway).

Does it matter? My understanding is Terra dropped 3.7 support, and several ecosystem projects have too.

docs/images/gallery_shot.png Outdated Show resolved Hide resolved
docs/release_notes.rst Outdated Show resolved Hide resolved
@jakelishman
Copy link
Member

Terra 0.24 still supports Python 3.7 and is deployed via the metapackage (for now), so we're still in the support window of 3.7 technically. That said, I'm happy if I don't have to put any effort into updating qiskit-qasm3-import for 3.7 during that window; we already judged it "acceptable" not to support 3.7 for an optional feature like that when it was added, since there are other optionals that have dropped support too.

Comment on lines -7572 to +7573
- Fixed support for running :meth:`.Z2Symmetries.taper` on larger problems.
- Fixed support for running ``Z2Symmetries.taper()`` on larger problems.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fyi @jakelishman these still lose cross references because I could not find the file in Terra. It's the ones that are notes/<foo>.yaml, whereas I could find the files for notes/<version>/<foo>.yaml.

Copy link
Member

Choose a reason for hiding this comment

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

That probably happens because we move the files in Terra from being in the root releasenotes/notes directory to a versioned directory as part of the minor-release procedure, but we don't tend to bother when doing patch releases on the stable branch; they only get moved later when we silo off the next minor release. We wouldn't bother going through the comments in the metapackage updating stuff.

I'm not sure why that causes us to lose the cross-references. Fwiw, that particular note will be referring to the Opflow version of Z2Symmetries - it's from before the quantum_info version was added - so while it's still a valid cross-reference for now, it won't be once Opflow is removed in a couple of versions' time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

'm not sure why that causes us to lose the cross-references.

Ah, I realize this wasn't clear. These cross-references are ambiguous so Sphinx has a warning it can't link properly.

The only way in other places I was able to disambiguate is by looking up in Terra what the updated release notes are. Those were already disambiguate in Terra, just not updated here.

For here and the few other references I had to remove, I both could not find the file name with fd nor some sample text from the release note, e.g. neither of these match this note:

❯ rg 'support for running'           
releasenotes/notes/0.20/ibm-cpu-arch-support-3289377f3834f29e.yaml
4:    Added partial support for running on ppc64le and s390x Linux platforms.

releasenotes/notes/0.16/drop-py3.5-86a65daa49903d46.yaml
4:    The deprecated support for running Qiskit Terra with Python 3.5 has been

Rather than trying to guess how to disambiguate, I removed the cross-reference.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's fair enough - I generally suggest that if the natural Sphinx references cause Sphinx to complain about ambiguity then there's a very high chance that the reader will be confused as well, so it's worth including more lead-in to the link (e.g. :meth:`.opflow.Z2Symmetries.taper`). That said, these are old and their targets are going to disappear in the not-too-distant future, so it's fine to just drop them now.

I'm not sure what fd program you're referring to, but the search fails because the release note was rewritten during its backporting (see Qiskit/qiskit#8410 and Qiskit/qiskit#8514) - looks like the note got merged to main in a not-great state, and Matthew rewrote it before it'd appear. Since patch-release notes are just hidden on main as part of the minor-release procedure, we don't always bother to bring any changes in.

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'm not sure what fd program you're referring to

fd is an amazing rewrite of find. Similar to rg. See https://github.com/unpluggedcoder/awesome-rust-tools

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

This looks good to merge to me. Do you need me to fix qiskit-qasm3-import in the end, or are we ok?

@Eric-Arellano
Copy link
Collaborator Author

Do you need me to fix qiskit-qasm3-import in the end, or are we ok?

No, we're okay. I reverted the change to tests dropping Python 3.7 support. Now, only the lint and docs jobs are upgraded to 3.8. And I used your conditional requirement idea. Thanks!

@Eric-Arellano Eric-Arellano merged commit c958ffc into Qiskit:master May 22, 2023
@Eric-Arellano Eric-Arellano deleted the fix-docs-issues branch May 22, 2023 18:18
jakelishman pushed a commit to jakelishman/qiskit-terra that referenced this pull request Aug 11, 2023
Fixes the majority of docs failures, which are broadly these categories:

* Invalid references
* Invalid Sphinx syntax
* Config out of sync with qiskit-terra, e.g. missing doctest and qasm3 requirements.

We can't yet enable `sphinx-build -W` because there are some bigger issues still:

* Stop using `sphinx-redirects`: Qiskit/qiskit-metapackage#1738
* Fix an issue in the qiskit-tutorials repo, which should also be setting `-W` in its CI.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants