Allow dedicated page for each method #543
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Dec 21, 2023
arnaucasau
added a commit
to arnaucasau/documentation
that referenced
this pull request
Dec 22, 2023
This reverts commit 9ce5f64.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 29, 2023
The generation script was not inlining all the methods in the class pages because we weren't assigning the correct value to the `python_api_name` metadata. With this PR, we change the way we search for the id we set as the name in the `sphinxHtmlToMarkdown.ts` The problem comes from we are missing the class `sig-object` in the `dt` tags we used to extract the `python_api_name` metadata in the HTML used to generate the markdown API files. We actually don't need to specify the class because the `dt` tag we are looking for is unique, and this PR fixes that. Moreover, the changes introduced in #543 are reverted to recover the mergeClassMembers.ts script that we need to inline all the methods in historical versions. Tested the generation of Qiskit 0.33, where we will inline the methods in a follow-up, and also the generation of Qiskit 0.45 to validate we are not modifying any version that was working previously. Commands used (Notice we need to have the API docs downloaded in .out): > npm run gen-api -- -p qiskit -v 0.33.0 -a "" --historical > npm run gen-api -- -p qiskit -v 0.45.0 -a "" Part of #542
frankharkins
pushed a commit
to frankharkins/documentation
that referenced
this pull request
Jul 22, 2024
Part of fixing Qiskit#542. ## Methods living on dedicated pages Before Qiskit 0.44, methods lived on dedicated HTML pages. We changed that in Qiskit 0.44 because it resulted in horrible build-time performance when migrating `qiskit.org/documentation` from the old Pytorch theme to the new Furo theme, so that instead methods lived directly on their class pages. It looks like the script `updateApiDocs.ts` was trying to emulate methods living on class pages before Qiskit 0.44 through the file `mergeClassMembers.ts`. This code attempted to remove the method pages and instead have them live directly on the class pages. I believe the original script author had `mergeClassMembers.ts` to try to avoid performance issues. Until a fix a few weeks ago to the docs application, having a huge amount of pages would cause the site to load extremely slowly due to the left side bar being too large. That is now fixed because the left side bar is loaded dynamically rather than statically. ## `mergeClassMembers.ts` was causing problems 1. It looks like it wasn't behaving properly, at least some of the time. We did not exhaustively audit every page, but for example, https://docs.quantum.ibm.com/api/qiskit/0.33/qiskit.result.Result only had properties at the top, then a table summarizing all the methods below, but not the methods actually inlined: <img width="774" alt="Screenshot 2023-12-21 at 11 43 47 AM" src="https://github.com/Qiskit/documentation/assets/14852634/576ff54a-763c-484d-bc3b-42068693b0d1"> 2. We didn't know about this inlining when setting up qiskit.org redirects, which do a 1:1 mapping so that dedicated method pages take you to the dedicated method page on IBM, which was 404ing. ## Solution: allow dedicated method pages By deleting `mergeClassMembers.ts`, the script will properly generate dedicated HTML pages for each method. We took this approach—rather than instead fixing `mergeClassMembers.ts`—for a few reasons: 1. It allows us to avoid messing with our redirect rules, which are the 1:1 mapping. 2. We've gotten user feedback that the API docs are not as usable as they could be. We are planning to fix that in Qiskit#479, but it won't be fixed in time for putting out the fire of Qiskit#542. In the meantime, it is more user-friendly to have dedicated method pages for those historical API docs. The biggest reason we would not want to take this approach of dedicated method pages is the possible impact on performance mentioned above. But that has been fixed already in the IBM application. ## Impact of this PR No impact when generating docs where methods already live on class pages, like Qiskit 0.44+. For earlier Qiskit versions, it results in the method pages being generated. Those will be added in follow up PRs. --------- Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
frankharkins
pushed a commit
to frankharkins/documentation
that referenced
this pull request
Jul 22, 2024
The generation script was not inlining all the methods in the class pages because we weren't assigning the correct value to the `python_api_name` metadata. With this PR, we change the way we search for the id we set as the name in the `sphinxHtmlToMarkdown.ts` The problem comes from we are missing the class `sig-object` in the `dt` tags we used to extract the `python_api_name` metadata in the HTML used to generate the markdown API files. We actually don't need to specify the class because the `dt` tag we are looking for is unique, and this PR fixes that. Moreover, the changes introduced in Qiskit#543 are reverted to recover the mergeClassMembers.ts script that we need to inline all the methods in historical versions. Tested the generation of Qiskit 0.33, where we will inline the methods in a follow-up, and also the generation of Qiskit 0.45 to validate we are not modifying any version that was working previously. Commands used (Notice we need to have the API docs downloaded in .out): > npm run gen-api -- -p qiskit -v 0.33.0 -a "" --historical > npm run gen-api -- -p qiskit -v 0.45.0 -a "" Part of Qiskit#542
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of fixing #542.
Methods living on dedicated pages
Before Qiskit 0.44, methods lived on dedicated HTML pages. We changed that in Qiskit 0.44 because it resulted in horrible build-time performance when migrating
qiskit.org/documentationfrom the old Pytorch theme to the new Furo theme, so that instead methods lived directly on their class pages.It looks like the script
updateApiDocs.tswas trying to emulate methods living on class pages before Qiskit 0.44 through the filemergeClassMembers.ts. This code attempted to remove the method pages and instead have them live directly on the class pages.I believe the original script author had
mergeClassMembers.tsto try to avoid performance issues. Until a fix a few weeks ago to the docs application, having a huge amount of pages would cause the site to load extremely slowly due to the left side bar being too large. That is now fixed because the left side bar is loaded dynamically rather than statically.mergeClassMembers.tswas causing problemsSolution: allow dedicated method pages
By deleting
mergeClassMembers.ts, the script will properly generate dedicated HTML pages for each method.We took this approach—rather than instead fixing
mergeClassMembers.ts—for a few reasons:The biggest reason we would not want to take this approach of dedicated method pages is the possible impact on performance mentioned above. But that has been fixed already in the IBM application.
Impact of this PR
No impact when generating docs where methods already live on class pages, like Qiskit 0.44+.
For earlier Qiskit versions, it results in the method pages being generated. Those will be added in follow up PRs.