Skip to content

Set up redirects for functions moved to module pages#10692

Merged
jakelishman merged 8 commits into
Qiskit:mainfrom
Eric-Arellano:redirect-api-docs
Aug 30, 2023
Merged

Set up redirects for functions moved to module pages#10692
jakelishman merged 8 commits into
Qiskit:mainfrom
Eric-Arellano:redirect-api-docs

Conversation

@Eric-Arellano
Copy link
Copy Markdown
Contributor

We've had a spike in 404s since landing #10455 and #10471. We were able to set up in Cloudflare redirects for methods because every single method now lives on its class page. But we can't use a generic rule for functions because some functions still have dedicated HTML pages.

So instead, we use https://documatt.gitlab.io/sphinx-reredirects for granular redirects. It will create a page for each redirect that has metadata to go to the new page. These redirect pages do not show up in the left sidebar.

Note that we used to use sphinx-reredirects but removed it in qiskit-metapackage because it was not setting that it's parallel safe, which slows down the Sphinx build. That has since been fixed.

I came up with the list in api_redirects.txt by looking at #10471 and #10522, and checking what the HTML pages are on the live site. That is, I did not automate this because there were too many edge cases.

@Eric-Arellano Eric-Arellano added documentation Something is not clear or an error documentation stable backport potential Make Mergify open a backport PR to the most recent stable branch on merge. Changelog: None Do not include in the GitHub Release changelog. labels Aug 22, 2023
@Eric-Arellano Eric-Arellano added this to the 0.25.2 milestone Aug 22, 2023
@jakelishman
Copy link
Copy Markdown
Member

jakelishman commented Aug 23, 2023

If I were you I might consider downloading the Sphinx inventory for the old version and the current stable version, then reading the old and new locations for each out of there; it'll automatically handle any edge cases in our structure because it's just referencing Sphinx's knowledge of where it put stuff. You can read a bit more about the syntax here: https://sphobjinv.readthedocs.io/en/stable/syntax.html.

@coveralls
Copy link
Copy Markdown

coveralls commented Aug 23, 2023

Pull Request Test Coverage Report for Build 5954921836

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 4 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.003%) to 87.283%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 93.76%
crates/qasm2/src/lex.rs 3 91.65%
Totals Coverage Status
Change from base Build 5952293521: 0.003%
Covered Lines: 74289
Relevant Lines: 85113

💛 - Coveralls

@Eric-Arellano
Copy link
Copy Markdown
Contributor Author

If I were you I might consider downloading the Sphinx inventory for the old version and the current stable version, then reading the old and new locations for each out of there;

Are you suggesting to dynamically generate the redirect values every PR? Or to generate the redirects one time for this PR?

Either way, this PR is now ready for review. I double checked in the built docs that redirects are working correctly, e.g. file:///Users/arellano/Downloads/qiskit-redirects2/stubs/qiskit.algorithms.eval_observables.html takes me to https://qiskit.org/documentation/apidoc/algorithms.html#qiskit.algorithms.eval_observables

@Eric-Arellano Eric-Arellano marked this pull request as ready for review August 23, 2023 13:48
@Eric-Arellano Eric-Arellano requested a review from a team as a code owner August 23, 2023 13:48
@qiskit-bot
Copy link
Copy Markdown
Collaborator

One or more of the the following people are requested to review this:

@jakelishman
Copy link
Copy Markdown
Member

I was suggesting to generate it locally and commit the generated file; the redirections are inherently static from one version to another particular one.

@mtreinish
Copy link
Copy Markdown
Member

mtreinish commented Aug 23, 2023

I was suggesting to generate it locally and commit the generated file; the redirections are inherently static from one version to another particular one.

This is what we did for rustworkx when we renamed stubs to apiref: https://github.com/Qiskit/rustworkx/blob/main/docs/source/sources.txt and https://github.com/Qiskit/rustworkx/blob/main/docs/source/conf.py#L122-L125

Edit: and looking at the PR is basically what is done here too

@Eric-Arellano
Copy link
Copy Markdown
Contributor Author

Edit: and looking at the PR is basically what is done here too

Oh, yeah. I only did it manually by looking in Git at what change we made this month across two PRs. Good to know for the future.

Comment thread docs/conf.py Outdated
Comment on lines +257 to +263
old_doc_name, new_module_page_name = line.split(" ")
obj_name = old_doc_name.split("stubs/")[1]
# E.g. `../apidoc/assembler.html#qiskit.assembler.assemble_circuits
result[old_doc_name] = (
"https://qiskit.org/documentation/apidoc/" +
f"{new_module_page_name}.html#{obj_name}"
)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This feels like it could have been done statically in the file, and would be a bit more explicit / easy to extend with more redirections if so. Despite the pages mentioned in the flat file all being moved from stubs to apidoc, we have stubs in the file, but apidoc hardcoded in the generation code. Personally, if we're going to have a static file, I'd consider just being fully explicit and having something like a three-column old_file, new_file, anchor? structure.

I'm also pretty nervous about hardcoding the deployment URL here - as an immediate effect, it means that local builds, and the dev and stable-branch deployments will all be broken, but as a general thing, Sphinx usually goes out of its way not to require this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

espite the pages mentioned in the flat file all being moved from stubs to apidoc, we have stubs in the file

Yeah, good point. I'll tweak.

I'm also pretty nervous about hardcoding the deployment URL here - as an immediate effect, it means that local builds, and the dev and stable-branch deployments will all be broken, but as a general thing, Sphinx usually goes out of its way not to require this.

So my original implementation tried using a relative file, which is what sphinx-reredirects suggests using: https://documatt.gitlab.io/sphinx-reredirects. But the redirects did not actually work when I tested out the build. I suspect it can't handle the anchors properly.

I noticed we used full URLs in qiskit-metapackage for the Aer redirects: https://github.com/Qiskit/qiskit-metapackage/pull/1742/files, which led me to try it here.

Re: local builds, agreed. That's unfortunate that this won't handle local builds properly. I reason that it's worth the tradeoff so that we can have anchors, though.

Re: dev and stable, I don't think we care much about redirects on those? Sphinx already handles fixing bad links with internal docs via its cross references. What we're trying to fix with this PR is external links, like YouTube. If those were using a stable version, then there's nothing broken because we only changed the organization with a new Qiskit version and didn't change prior stable versions. We want to handle unstable URLs like https://qiskit.org/stubs/qiskit.algorithms.eval_observables.html.

Comment thread docs/conf.py
Comment on lines +247 to +250

Note that we have redirects in Cloudflare for methods moving to their class page. We
could not do this for functions because some functions still have dedicated
HTML pages, so we cannot use a generic rule.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

tbh I think it'd be easier to manage if we did it all locally here, but it's not super necessary. It's not as pleasant to use HTML meta redirects rather than proper server-side 301 responses, but it's much easier for us to generate the lists here so the documentation build can be reproducible without external configuration. If it's a resolved static list as well, it also would (presumably) be easier to sync it into the cloudflare config if necessary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We're using Cloudflare for a lot:

  • all the Ecosystem redirects for what used to be documentation/<project>
  • The Aer API docs that used to be under Qiskit's API docs. We had two migrations here
  • As of yesterday, Qiskit methods now going to class pages
  • several tutorials migrations
  • Textbook/learning platform stuff

Point being, we will need to keep the Cloudflare redirects for qiskit.org in general and it's impossible for sphinx-reredirects to handle it all since it only helps with the /documentation subsite. So, I don't see much reason to move some of Cloudflare here.

Comment thread docs/api_redirects.txt Outdated
@Eric-Arellano
Copy link
Copy Markdown
Contributor Author

@jakelishman I wrote this script to check for typos:

from pathlib import Path
import requests

lines = Path("docs/api_redirects.txt").read_text().splitlines()
old_urls = []
new_urls = set()
for line in lines:
    if not line:
        continue
    obj_name, new_module_page_name = line.split(" ")
    old_urls.append(f"https://qiskit.org/documentation/stable/0.43/stubs/{obj_name}.html")
    new_urls.add(f"https://qiskit.org/documentation/apidoc/{new_module_page_name}.html")


def check_url(url: str, desc: str) -> None:
    try:
        response = requests.get(url, timeout=10)
    except requests.RequestException as e:
        print(f"Error: {e}")
    else:
        if response.status_code == 404:
            print(f"Bad {desc}: {url}")


for new_url in new_urls:
    check_url(new_url, "new url")
for old_url in old_urls:
    check_url(old_url, "old url")

It found 4 issues that I fixed with the latest commit. Good call to be careful on typos.

Copy link
Copy Markdown
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 the getting the typos and whatnot fixed - let's just get this merged to fix the 404s (sorry for the delay).

@jakelishman jakelishman added this pull request to the merge queue Aug 30, 2023
@1ucian0 1ucian0 self-assigned this Aug 30, 2023
Merged via the queue into Qiskit:main with commit adc1d4c Aug 30, 2023
mergify Bot pushed a commit that referenced this pull request Aug 30, 2023
* Set up redirects for functions moved to module pages

* Add exceptions from Jake's PR

* Set up conf.py

* Try full URLs for redirects

* DRY stubs/ in api_redirects.txt

* Fix typo

* Fix issues found by 404 script

(cherry picked from commit adc1d4c)

# Conflicts:
#	docs/conf.py
#	requirements-dev.txt
@Eric-Arellano Eric-Arellano deleted the redirect-api-docs branch August 30, 2023 16:58
github-merge-queue Bot pushed a commit that referenced this pull request Aug 31, 2023
#10738)

* Set up redirects for functions moved to module pages (#10692)

* Set up redirects for functions moved to module pages

* Add exceptions from Jake's PR

* Set up conf.py

* Try full URLs for redirects

* DRY stubs/ in api_redirects.txt

* Fix typo

* Fix issues found by 404 script

(cherry picked from commit adc1d4c)

# Conflicts:
#	docs/conf.py
#	requirements-dev.txt

* Fix merge conflicts

---------

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changelog: None Do not include in the GitHub Release changelog. documentation Something is not clear or an error documentation stable backport potential Make Mergify open a backport PR to the most recent stable branch on merge.

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants