Use `sphinx.ext.linkcode` for more precise source code links #11851

melechlapson · 2024-02-21T16:29:13Z

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 like stable/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 in conf.py. You can see the workflow correctly passing the value to Sphinx in this example run.

Examples of the workflow logic:

❯ GITHUB_REF_NAME="1.0.1rc1" bash test.sh  # tag
Using branch 'stable/1.0' for GitHub source code links
❯ GITHUB_REF_NAME="stable/0.2" bash test.sh     # branch build   
Using branch 'stable/0.2' for GitHub source code links
❯ GITHUB_BASE_REF=main bash test.sh  # PR
Using branch 'main' for GitHub source code links

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.

CLAassistant · 2024-02-21T16:29:19Z

All committers have signed the CLA.

CLAassistant · 2024-02-21T16:29:19Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

docs/conf.py

Co-authored-by: Eric Arellano <[email protected]>

…b.com/melechlapson/qiskit into mlapson/switch-to-sphinx-ext-linkcode

Co-authored-by: Eric Arellano <[email protected]>

docs/conf.py

Co-authored-by: Eric Arellano <[email protected]>

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

mtreinish · 2024-03-18T11:24:08Z

docs/conf.py

+        is_valid_code_object = (
+            inspect.isclass(obj) or inspect.ismethod(obj) or inspect.isfunction(obj)
+        )
+        if not is_valid_code_object:
+            return None


How does this work for attributes? Does it not matter because sphinx doesn't generate source links for documented attributes of a class.

Yeah, attributes are skipped.

.github/workflows/docs_deploy.yml

docs/conf.py

Co-authored-by: Eric Arellano <[email protected]>

jakelishman

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.

.github/workflows/docs_deploy.yml

jakelishman · 2024-04-04T11:45:21Z

docs/conf.py

+    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]


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.

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).

Thanks for the suggestion!

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.

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.

.github/workflows/docs_deploy.yml

…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

jakelishman

Thanks for the changes, this seems fine to me.

Eric-Arellano · 2024-04-05T13:37:38Z

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

jakelishman

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 <[email protected]> * removed docstring Co-authored-by: Eric Arellano <[email protected]> * update return string Co-authored-by: Eric Arellano <[email protected]> * added back blank line * Added a method to determine the GitHub branch Co-authored-by: Eric Arellano <[email protected]> * add blank line Co-authored-by: Eric Arellano <[email protected]> * remove print statement Co-authored-by: Eric Arellano <[email protected]> * 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 <[email protected]> * 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 <[email protected]> (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 <[email protected]> * removed docstring Co-authored-by: Eric Arellano <[email protected]> * update return string Co-authored-by: Eric Arellano <[email protected]> * added back blank line * Added a method to determine the GitHub branch Co-authored-by: Eric Arellano <[email protected]> * add blank line Co-authored-by: Eric Arellano <[email protected]> * remove print statement Co-authored-by: Eric Arellano <[email protected]> * 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 <[email protected]> * 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 <[email protected]> (cherry picked from commit 97788f0) Co-authored-by: melechlapson <[email protected]>

melechlapson added 2 commits February 20, 2024 17:03

switched from sphinx.ext.viewcode to sphinx.ext.linkcode

099636d

removed extra line

00b05e7

qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Feb 21, 2024

Merge branch 'main' into mlapson/switch-to-sphinx-ext-linkcode

a20ad5c

Eric-Arellano reviewed Feb 21, 2024

View reviewed changes

docs/conf.py Show resolved Hide resolved

docs/conf.py Outdated Show resolved Hide resolved

docs/conf.py Outdated Show resolved Hide resolved

docs/conf.py Outdated Show resolved Hide resolved

melechlapson and others added 5 commits February 21, 2024 14:48

Add section header for source code links

2f94288

Co-authored-by: Eric Arellano <[email protected]>

removed docstring

1d5c34d

Co-authored-by: Eric Arellano <[email protected]>

update return string

1849ce1

Co-authored-by: Eric Arellano <[email protected]>

added back blank line

cca9b04

Merge branch 'mlapson/switch-to-sphinx-ext-linkcode' of https://githu…

5bd18cd

…b.com/melechlapson/qiskit into mlapson/switch-to-sphinx-ext-linkcode

Eric-Arellano changed the title ~~WIP - Switching Qiskit documentation from using sphinx.ext.viewcode to sphinx.ext.linkcode extension~~ WIP - Switch Qiskit documentation from using sphinx.ext.viewcode to sphinx.ext.linkcode extension Feb 21, 2024

Eric-Arellano added documentation Something is not clear or an error documentation stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: None Do not include in changelog labels Feb 21, 2024

Eric-Arellano changed the title ~~WIP - Switch Qiskit documentation from using sphinx.ext.viewcode to sphinx.ext.linkcode extension~~ WIP - Use sphinx.ext.linkcode for more precise source code links Feb 21, 2024

Added a method to determine the GitHub branch

b985ba6

Co-authored-by: Eric Arellano <[email protected]>

melechlapson force-pushed the mlapson/switch-to-sphinx-ext-linkcode branch from d58d8ba to b985ba6 Compare February 21, 2024 21:10

melechlapson changed the title ~~WIP - Use sphinx.ext.linkcode for more precise source code links~~ Use sphinx.ext.linkcode for more precise source code links Feb 21, 2024

melechlapson marked this pull request as ready for review February 21, 2024 21:13

melechlapson requested a review from a team as a code owner February 21, 2024 21:13

This comment was marked as outdated.

Sign in to view

Merge branch 'main' into mlapson/switch-to-sphinx-ext-linkcode

914d2cd

Eric-Arellano reviewed Feb 21, 2024

View reviewed changes

docs/conf.py Outdated Show resolved Hide resolved

docs/conf.py Outdated Show resolved Hide resolved

melechlapson and others added 2 commits February 21, 2024 15:16

add blank line

1deb939

Co-authored-by: Eric Arellano <[email protected]>

remove print statement

b84b399

Co-authored-by: Eric Arellano <[email protected]>

Eric-Arellano changed the title ~~Use sphinx.ext.linkcode for more precise source code links~~ [wip] Use sphinx.ext.linkcode for more precise source code links Feb 21, 2024

Eric-Arellano marked this pull request as draft February 21, 2024 21:57

Eric-Arellano added 3 commits March 7, 2024 15:54

Merge branch 'main' of github.com:Qiskit/qiskit-terra into mlapson/sw…

51f4bcb

…itch-to-sphinx-ext-linkcode

Move GitHub branch logic to GitHub Action

ab7f411

Merge branch 'mlapson/switch-to-sphinx-ext-linkcode' of github.com:me…

2a28edb

…lechlapson/qiskit into mlapson/switch-to-sphinx-ext-linkcode

Eric-Arellano changed the title ~~[wip] Use sphinx.ext.linkcode for more precise source code links~~ Use sphinx.ext.linkcode for more precise source code links Mar 7, 2024

mtreinish reviewed Mar 18, 2024

View reviewed changes

switched to importlib and removed redundant block of code

cde30ef

Eric-Arellano reviewed Mar 21, 2024

View reviewed changes

docs/conf.py Outdated Show resolved Hide resolved

docs/conf.py Outdated Show resolved Hide resolved

melechlapson and others added 2 commits March 21, 2024 09:59

Apply suggestions from code review

5545bc8

Co-authored-by: Eric Arellano <[email protected]>

Merge branch 'main' into mlapson/switch-to-sphinx-ext-linkcode

7c95384

jakelishman reviewed Apr 4, 2024

View reviewed changes

melechlapson and others added 4 commits April 4, 2024 09:32

added back spaces

5b74a03

Merge branch 'main' of github.com:Qiskit/qiskit-terra into mlapson/sw…

8a805da

…itch-to-sphinx-ext-linkcode

Clarify docs_deploy GitHub logic

abf4871

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

3386c4c

jakelishman previously approved these changes Apr 5, 2024

View reviewed changes

jakelishman added this pull request to the merge queue Apr 5, 2024

Eric-Arellano removed this pull request from the merge queue due to a manual request Apr 5, 2024

Fix relative_to() path

614e8f8

Eric-Arellano dismissed jakelishman’s stale review via 614e8f8 April 5, 2024 12:16

Remove tox prefix

19d052d

jakelishman approved these changes Apr 5, 2024

View reviewed changes

jakelishman added this pull request to the merge queue Apr 5, 2024

Merged via the queue into Qiskit:main with commit 97788f0 Apr 5, 2024
12 checks passed

mergify bot mentioned this pull request Apr 5, 2024

Use sphinx.ext.linkcode for more precise source code links (backport #11851) #12146

Merged

This was referenced Apr 15, 2024

Use sphinx.ext.linkcode for more precise source code links Qiskit/qiskit-aer#2101

Merged

Change source code links to go to GitHub qiskit-community/qiskit-algorithms#170

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `sphinx.ext.linkcode` for more precise source code links #11851

Use `sphinx.ext.linkcode` for more precise source code links #11851

melechlapson commented Feb 21, 2024 •

edited by Eric-Arellano

Loading

CLAassistant commented Feb 21, 2024 •

edited

Loading

CLAassistant commented Feb 21, 2024

This comment was marked as outdated.

mtreinish Mar 18, 2024

Eric-Arellano Mar 18, 2024

jakelishman left a comment

jakelishman Apr 4, 2024

jakelishman Apr 4, 2024

Eric-Arellano Apr 4, 2024

Eric-Arellano Apr 5, 2024

jakelishman Apr 5, 2024

jakelishman left a comment

Eric-Arellano commented Apr 5, 2024 •

edited

Loading

jakelishman left a comment

Use sphinx.ext.linkcode for more precise source code links #11851

Use sphinx.ext.linkcode for more precise source code links #11851

Conversation

melechlapson commented Feb 21, 2024 • edited by Eric-Arellano Loading

Summary

Determining the GitHub branch

PR builds use main

CLAassistant commented Feb 21, 2024 • edited Loading

CLAassistant commented Feb 21, 2024

This comment was marked as outdated.

mtreinish Mar 18, 2024

Choose a reason for hiding this comment

Eric-Arellano Mar 18, 2024

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

jakelishman Apr 4, 2024

Choose a reason for hiding this comment

jakelishman Apr 4, 2024

Choose a reason for hiding this comment

Eric-Arellano Apr 4, 2024

Choose a reason for hiding this comment

Eric-Arellano Apr 5, 2024

Choose a reason for hiding this comment

jakelishman Apr 5, 2024

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

Eric-Arellano commented Apr 5, 2024 • edited Loading

jakelishman left a comment

Choose a reason for hiding this comment

Use `sphinx.ext.linkcode` for more precise source code links #11851

Use `sphinx.ext.linkcode` for more precise source code links #11851

melechlapson commented Feb 21, 2024 •

edited by Eric-Arellano

Loading

PR builds use `main`

CLAassistant commented Feb 21, 2024 •

edited

Loading

Eric-Arellano commented Apr 5, 2024 •

edited

Loading