From 9ce5f6422d9cd59b5311c3bc5752236ce21b00e9 Mon Sep 17 00:00:00 2001 From: Arnau Casau <47946624+arnaucasau@users.noreply.github.com> Date: Thu, 21 Dec 2023 18:01:06 +0100 Subject: [PATCH] Allow dedicated page for each method (#543) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part of fixing https://github.com/Qiskit/documentation/issues/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: Screenshot 2023-12-21 at 11 43 47 AM 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 https://github.com/Qiskit/documentation/issues/479, but it won't be fixed in time for putting out the fire of https://github.com/Qiskit/documentation/issues/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> --- scripts/commands/updateApiDocs.ts | 2 - scripts/lib/sphinx/mergeClassMembers.test.ts | 131 ---------------- scripts/lib/sphinx/mergeClassMembers.ts | 154 ------------------- 3 files changed, 287 deletions(-) delete mode 100644 scripts/lib/sphinx/mergeClassMembers.test.ts delete mode 100644 scripts/lib/sphinx/mergeClassMembers.ts diff --git a/scripts/commands/updateApiDocs.ts b/scripts/commands/updateApiDocs.ts index 175caea6051..c7ec16089d4 100644 --- a/scripts/commands/updateApiDocs.ts +++ b/scripts/commands/updateApiDocs.ts @@ -23,7 +23,6 @@ import { WebCrawler } from "../lib/WebCrawler"; import { downloadImages } from "../lib/downloadImages"; import { generateToc } from "../lib/sphinx/generateToc"; import { SphinxToMdResult } from "../lib/sphinx/SphinxToMdResult"; -import { mergeClassMembers } from "../lib/sphinx/mergeClassMembers"; import { flatFolders } from "../lib/sphinx/flatFolders"; import { updateLinks } from "../lib/sphinx/updateLinks"; import { renameUrls } from "../lib/sphinx/renameUrls"; @@ -309,7 +308,6 @@ async function convertHtmlToMarkdown( await mkdirp(dir); } - results = await mergeClassMembers(results); flatFolders(results); renameUrls(results); await updateLinks(results, pkg.transformLink); diff --git a/scripts/lib/sphinx/mergeClassMembers.test.ts b/scripts/lib/sphinx/mergeClassMembers.test.ts deleted file mode 100644 index f6e383217f6..00000000000 --- a/scripts/lib/sphinx/mergeClassMembers.test.ts +++ /dev/null @@ -1,131 +0,0 @@ -// This code is a Qiskit project. -// -// (C) Copyright IBM 2023. -// -// This code is licensed under the Apache License, Version 2.0. You may -// obtain a copy of this license in the LICENSE file in the root directory -// of this source tree or at http://www.apache.org/licenses/LICENSE-2.0. -// -// Any modifications or derivative works of this code must retain this -// copyright notice, and modified files need to carry a notice indicating -// that they have been altered from the originals. - -import { describe, expect, test } from "@jest/globals"; -import { mergeClassMembers } from "./mergeClassMembers"; - -describe("mergeClassMembers", () => { - test("merge class members", async () => { - const results: Parameters[0] = [ - { - markdown: `## Attributes - -| | | -| ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------- | -| [\`RuntimeOptions.backend\`](qiskit_ibm_runtime.RuntimeOptions.backend#qiskit_ibm_runtime.RuntimeOptions.backend "qiskit_ibm_runtime.RuntimeOptions.backend") | | - -## Methods - -| | | -| ------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ----------------- | -| [\`RuntimeOptions.validate\`](qiskit_ibm_runtime.RuntimeOptions.validate#qiskit_ibm_runtime.RuntimeOptions.validate "qiskit_ibm_runtime.RuntimeOptions.validate")(channel) | Validate options. |`, - meta: { - python_api_type: "class", - python_api_name: "RuntimeOptions", - }, - url: "/docs/api/qiskit-ibm-runtime/stubs/qiskit_ibm_runtime.RuntimeOptions", - images: [], - isReleaseNotes: false, - }, - { - markdown: `# RuntimeOptions.backend - -\`Optional[str] = None\` -`, - meta: { - python_api_type: "attribute", - python_api_name: "RuntimeOptions.backend", - }, - url: "/docs/api/qiskit-ibm-runtime/stubs/qiskit_ibm_runtime.RuntimeOptions.backend", - images: [], - isReleaseNotes: false, - }, - { - markdown: `# RuntimeOptions.circuits - -\`Optional[str] = None\` -`, - meta: { - python_api_type: "property", - python_api_name: "RuntimeOptions.circuits", - }, - url: "/docs/api/qiskit-ibm-runtime/stubs/qiskit_ibm_runtime.RuntimeOptions.circuits", - images: [], - isReleaseNotes: false, - }, - { - markdown: ` -# RuntimeOptions.validate - -\`RuntimeOptions.validate(channel)\` - -Validate options. - -* Parameters: - - **channel** (\`str\`) – channel type. - -* Raises: - - **IBMInputValueError** – If one or more option is invalid. - -* Return type: - - \`None\` - `, - meta: { - python_api_type: "method", - python_api_name: "RuntimeOptions.backend", - }, - url: "/docs/api/qiskit-ibm-runtime/stubs/qiskit_ibm_runtime.RuntimeOptions.validate", - images: [], - isReleaseNotes: false, - }, - ]; - const merged = await mergeClassMembers(results); - expect( - merged.find((item) => item.meta.python_api_type === "class")?.markdown, - ).toMatchInlineSnapshot(` - "## Attributes - - ### RuntimeOptions.backend - - \`Optional[str] = None\` - - ### RuntimeOptions.circuits - - \`Optional[str] = None\` - - ## Methods - - ### RuntimeOptions.validate - - \`RuntimeOptions.validate(channel)\` - - Validate options. - - * Parameters: - - **channel** (\`str\`) – channel type. - - * Raises: - - **IBMInputValueError** – If one or more option is invalid. - - * Return type: - - \`None\` - " - `); - expect(merged.length).toEqual(1); - }); -}); diff --git a/scripts/lib/sphinx/mergeClassMembers.ts b/scripts/lib/sphinx/mergeClassMembers.ts deleted file mode 100644 index 6493eb476f0..00000000000 --- a/scripts/lib/sphinx/mergeClassMembers.ts +++ /dev/null @@ -1,154 +0,0 @@ -// This code is a Qiskit project. -// -// (C) Copyright IBM 2023. -// -// This code is licensed under the Apache License, Version 2.0. You may -// obtain a copy of this license in the LICENSE file in the root directory -// of this source tree or at http://www.apache.org/licenses/LICENSE-2.0. -// -// Any modifications or derivative works of this code must retain this -// copyright notice, and modified files need to carry a notice indicating -// that they have been altered from the originals. - -import { includes, isEmpty, orderBy, reject } from "lodash"; -import { unified } from "unified"; -import remarkParse from "remark-parse"; -import remarkMdx from "remark-mdx"; -import remarkGfm from "remark-gfm"; -import remarkMath from "remark-math"; -import remarkStringify from "remark-stringify"; -import { Content, Root } from "mdast"; -import { visit } from "unist-util-visit"; -import { SphinxToMdResultWithUrl } from "./SphinxToMdResult"; -import { remarkStringifyOptions } from "./unifiedParser"; - -export async function mergeClassMembers( - results: T[], -): Promise { - const resultsWithName = results.filter( - (result) => !isEmpty(result.meta.python_api_name), - ); - const classes = resultsWithName.filter( - (result) => result.meta.python_api_type === "class", - ); - - for (const clazz of classes) { - const members = orderBy( - resultsWithName.filter((result) => { - if ( - !includes( - ["method", "property", "attribute", "function"], - result.meta.python_api_type, - ) - ) - return false; - return result.meta.python_api_name?.startsWith( - `${clazz.meta.python_api_name}.`, - ); - }), - (result) => result.meta.python_api_name, - ); - - const attributesAndProps = members.filter( - (member) => - member.meta.python_api_type === "attribute" || - member.meta.python_api_type === "property", - ); - const methods = members.filter( - (member) => member.meta.python_api_type === "method", - ); - - try { - // inject members markdown - clazz.markdown = ( - await unified() - .use(remarkParse) - .use(remarkMdx) - .use(remarkGfm) - .use(remarkMath) - .use(() => { - return async (tree) => { - for (const node of tree.children) { - await replaceMembersAfterTitle( - tree, - node, - "Attributes", - attributesAndProps, - ); - await replaceMembersAfterTitle(tree, node, "Methods", methods); - await replaceMembersAfterTitle( - tree, - node, - "Methods Defined Here", - methods, - ); - } - }; - }) - .use(remarkStringify, remarkStringifyOptions) - .process(clazz.markdown) - ).toString(); - } catch (e) { - console.log("Error found in", clazz.meta.python_api_name); - console.log(clazz.markdown); - throw e; - } - } - - // remove merged results - const finalResults = reject(results, (result) => - includes(["method", "attribute", "property"], result.meta.python_api_type), - ); - - return finalResults; -} - -async function replaceMembersAfterTitle( - tree: Root, - node: Content, - title: string, - members: SphinxToMdResultWithUrl[], -) { - if (node.type !== "heading") return; - const nodeIndex = tree.children.indexOf(node); - if (nodeIndex === -1) return; - - const nextNode = tree.children[nodeIndex + 1]; - const firstChild = node.children[0]; - - if ( - firstChild?.type === "text" && - firstChild?.value === title && - nextNode?.type === "table" - ) { - const children: any[] = []; - for (const member of members) { - const updated = await parseMarkdownIncreasingHeading(member.markdown, 2); - children.push(...updated.children); - } - tree.children.splice(nodeIndex + 1, 1, ...children); - } -} - -async function parseMarkdownIncreasingHeading( - md: string, - depthIncrease: number, -): Promise { - const root = await unified() - .use(remarkParse) - .use(remarkGfm) - .use(remarkMath) - .use(remarkMdx) - .parse(md); - const changedTree = await unified() - .use(remarkMath) - .use(remarkGfm) - .use(remarkMdx) - .use(() => (root) => { - visit(root, "heading", (node: any) => { - node.depth = node.depth + depthIncrease; - }); - }) - .run(root); - return changedTree as Root; -}