-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Use sphinx.ext.linkcode
for more precise source code links
#11851
Use sphinx.ext.linkcode
for more precise source code links
#11851
Conversation
|
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
…b.com/melechlapson/qiskit into mlapson/switch-to-sphinx-ext-linkcode
sphinx.ext.linkcode
for more precise source code links
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
d58d8ba
to
b985ba6
Compare
sphinx.ext.linkcode
for more precise source code linkssphinx.ext.linkcode
for more precise source code links
This comment was marked as outdated.
This comment was marked as outdated.
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
sphinx.ext.linkcode
for more precise source code linkssphinx.ext.linkcode
for more precise source code links
We got this stacktrace: Traceback (most recent call last): File "/home/vsts/work/1/s/.tox/docs/lib/python3.8/site-packages/sphinx/events.py", line 96, in emit results.append(listener.handler(self.app, *args)) File "/home/vsts/work/1/s/.tox/docs/lib/python3.8/site-packages/sphinx/ext/linkcode.py", line 55, in doctree_read uri = resolve_target(domain, info) File "/home/vsts/work/1/s/docs/conf.py", line 216, in linkcode_resolve file_name = PurePath(full_file_name).relative_to(repo_root) File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/pathlib.py", line 908, in relative_to raise ValueError("{!r} does not start with {!r}" ValueError: '/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/contextlib.py' does not start with '/home/vsts/work/1/s' We shouldn't even attempt to generate a link for the file contextlib.py
…itch-to-sphinx-ext-linkcode
…lechlapson/qiskit into mlapson/switch-to-sphinx-ext-linkcode
sphinx.ext.linkcode
for more precise source code linkssphinx.ext.linkcode
for more precise source code links
docs/conf.py
Outdated
is_valid_code_object = ( | ||
inspect.isclass(obj) or inspect.ismethod(obj) or inspect.isfunction(obj) | ||
) | ||
if not is_valid_code_object: | ||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work for attributes? Does it not matter because sphinx doesn't generate source links for documented attributes of a class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, attributes are skipped.
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, and sorry about the delay. I just left a couple of minor nits/questions in line, but otherwise I'm happy to move to merge.
docs/conf.py
Outdated
try: | ||
full_file_name = inspect.getsourcefile(obj) | ||
except TypeError: | ||
return None | ||
if full_file_name is None: | ||
return None | ||
file_name = full_file_name.split("/qiskit/")[-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's quite unlikely to matter, but you might prefer to do this full_file_name.split
thing using the proper os.path
or pathlib
tooling. Something like:
from pathlib import Path
REPO_ROOT = Path(__file__).resolve().parents[1]
def linkcode_resolve(domain, info):
# ...
file_name = str(Path(full_file_name).resolve().relative_to(REPO_ROOT / "qiskit"))
The current form would break if we were ever to add a subdirectory of the qiskit
package that itself was called qiskit
- unlikely, but not impossible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, actually not even unlikely - I just remembered that I literally proposed doing that in #10737 (though in that case, there wouldn't be any Python-documentable objects in it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I now remember why we were using [-1]
. This current code results in the relative file path .tox/docs/lib/python3.8/site-packages/qiskit/circuit/classicalfunction/exceptions.py
.
I'm unclear why that is: I couldn't reproduce in qiskit_sphinx_theme
with setting in tox.ini
isolated_build = true
and usedevelop = false
to mirror Qiskit. (I've been having issues running tox locally in Qiskit due to autodoc). In qiskit_sphinx_theme
locally and GitHub Actions, the path is the normal file path you'd expect without the .tox
prefix. So I think we want this code to support both styles.
We could use a regex replace to remove the .tox
prefix if it's there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, perhaps you want to use import qiskit; qiskit.__file__
as the file-path root, so it'll automatically find the base of where the files are installed?
I don't mind a huge amount, though.
…itch-to-sphinx-ext-linkcode
1. Remove misleading PR conditional. This worfklow doesn't even run in PRs. It was bad copy-pasta from the runtime repo 2. Clarify why we point tag builds to their stable branch name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes, this seems fine to me.
This is now generating correct links in the PR preview, e.g. https://github.com/Qiskit/qiskit/blob/main/qiskit/circuit/gate.py#L169-L226. Should be ready to merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo #11851 (comment) is less fragile than the regex path-match here, but also, as long as it's working for you right now, I can't see the regex replace actually breaking the docs build, so I'm ok just to merge things
* switched from sphinx.ext.viewcode to sphinx.ext.linkcode * removed extra line * Add section header for source code links Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> * removed docstring Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> * update return string Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> * added back blank line * Added a method to determine the GitHub branch Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> * add blank line Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> * remove print statement Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> * Try to fix error for contextlib file We got this stacktrace: Traceback (most recent call last): File "/home/vsts/work/1/s/.tox/docs/lib/python3.8/site-packages/sphinx/events.py", line 96, in emit results.append(listener.handler(self.app, *args)) File "/home/vsts/work/1/s/.tox/docs/lib/python3.8/site-packages/sphinx/ext/linkcode.py", line 55, in doctree_read uri = resolve_target(domain, info) File "/home/vsts/work/1/s/docs/conf.py", line 216, in linkcode_resolve file_name = PurePath(full_file_name).relative_to(repo_root) File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/pathlib.py", line 908, in relative_to raise ValueError("{!r} does not start with {!r}" ValueError: '/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/contextlib.py' does not start with '/home/vsts/work/1/s' We shouldn't even attempt to generate a link for the file contextlib.py * Try to fix error for Jenkins run #20240221.52 New build failed with this stacktrace: Traceback (most recent call last): File "/home/vsts/work/1/s/.tox/docs/lib/python3.8/site-packages/sphinx/events.py", line 96, in emit results.append(listener.handler(self.app, *args)) File "/home/vsts/work/1/s/.tox/docs/lib/python3.8/site-packages/sphinx/ext/linkcode.py", line 55, in doctree_read uri = resolve_target(domain, info) File "/home/vsts/work/1/s/docs/conf.py", line 215, in linkcode_resolve full_file_name = inspect.getsourcefile(obj) File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/inspect.py", line 696, in getsourcefile filename = getfile(object) File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/inspect.py", line 665, in getfile raise TypeError('{!r} is a built-in class'.format(object)) TypeError: <class 'builtins.CustomClassical'> is a built-in class So I added a condition that the obj should not be a built-in * Check that qiskit in module name sooner * moved valid code object verification earlier * added try except statement to getattr call * added extra try/except block * Also support Azure Pipelines * removed unused import * Revert Azure support to keep things simple * added extra "/" to final URL * Move GitHub branch logic to GitHub Action * switched to importlib and removed redundant block of code * Apply suggestions from code review Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> * added back spaces * Clarify docs_deploy GitHub logic 1. Remove misleading PR conditional. This worfklow doesn't even run in PRs. It was bad copy-pasta from the runtime repo 2. Clarify why we point tag builds to their stable branch name * Use pathlib for relativizing file name * Fix relative_to() path * Remove tox prefix --------- Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> (cherry picked from commit 97788f0)
…#12146) * switched from sphinx.ext.viewcode to sphinx.ext.linkcode * removed extra line * Add section header for source code links Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> * removed docstring Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> * update return string Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> * added back blank line * Added a method to determine the GitHub branch Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> * add blank line Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> * remove print statement Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> * Try to fix error for contextlib file We got this stacktrace: Traceback (most recent call last): File "/home/vsts/work/1/s/.tox/docs/lib/python3.8/site-packages/sphinx/events.py", line 96, in emit results.append(listener.handler(self.app, *args)) File "/home/vsts/work/1/s/.tox/docs/lib/python3.8/site-packages/sphinx/ext/linkcode.py", line 55, in doctree_read uri = resolve_target(domain, info) File "/home/vsts/work/1/s/docs/conf.py", line 216, in linkcode_resolve file_name = PurePath(full_file_name).relative_to(repo_root) File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/pathlib.py", line 908, in relative_to raise ValueError("{!r} does not start with {!r}" ValueError: '/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/contextlib.py' does not start with '/home/vsts/work/1/s' We shouldn't even attempt to generate a link for the file contextlib.py * Try to fix error for Jenkins run #20240221.52 New build failed with this stacktrace: Traceback (most recent call last): File "/home/vsts/work/1/s/.tox/docs/lib/python3.8/site-packages/sphinx/events.py", line 96, in emit results.append(listener.handler(self.app, *args)) File "/home/vsts/work/1/s/.tox/docs/lib/python3.8/site-packages/sphinx/ext/linkcode.py", line 55, in doctree_read uri = resolve_target(domain, info) File "/home/vsts/work/1/s/docs/conf.py", line 215, in linkcode_resolve full_file_name = inspect.getsourcefile(obj) File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/inspect.py", line 696, in getsourcefile filename = getfile(object) File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/inspect.py", line 665, in getfile raise TypeError('{!r} is a built-in class'.format(object)) TypeError: <class 'builtins.CustomClassical'> is a built-in class So I added a condition that the obj should not be a built-in * Check that qiskit in module name sooner * moved valid code object verification earlier * added try except statement to getattr call * added extra try/except block * Also support Azure Pipelines * removed unused import * Revert Azure support to keep things simple * added extra "/" to final URL * Move GitHub branch logic to GitHub Action * switched to importlib and removed redundant block of code * Apply suggestions from code review Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> * added back spaces * Clarify docs_deploy GitHub logic 1. Remove misleading PR conditional. This worfklow doesn't even run in PRs. It was bad copy-pasta from the runtime repo 2. Clarify why we point tag builds to their stable branch name * Use pathlib for relativizing file name * Fix relative_to() path * Remove tox prefix --------- Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com> (cherry picked from commit 97788f0) Co-authored-by: melechlapson <melechlapson@yahoo.com>
Summary
This addresses Qiskit/documentation#517 by switching out the sphinx.ext.viewcode for the sphinx.ext.linkcode extension which allows our GitHub links in the documentation to be linked to the specific lines of code, not just the file. This was tested successfully in Qiskit/qiskit_sphinx_theme#589 so now we want to implement it in the qiskit, runtime, and provider repos.
Example links generated in this PR build:
The API generation script at qiskit/documentation already knows how to handle
sphinx.ext.linkcode
.Determining the GitHub branch
We need to detect when to use
main
vs stable branches likestable/1.0
, particularly for stable docs builds with GitHub Actions that get used by qiskit/documentation to deploy the docs. PR and local builds are less critical because the docs don't get published and are only for previews by code authors.We do this with GitHub Actions environment variables. The docs workflow sets the env variable
QISKIT_DOCS_GITHUB_BRANCH_NAME
read by Sphinx inconf.py
. You can see the workflow correctly passing the value to Sphinx in this example run.Examples of the workflow logic:
PR builds use
main
PR builds and the merge queue use Azure Pipelines. To keep this infrastructure simpler, we simply default to
main
in PR builds, rather than adding Azure support.This means the URL will go to the wrong branch in docs previews for PRs against stable branches, although the stable branch build will work correctly. I don't expect this to matter much because PR builds are solely for docs previews; I'm skeptical code authors will be opening up the
[source]
link to double check the line numbers are exactly correct, since that line number calculation is 100% automated.