Handle Qiskit release notes structure correctly #537

arnaucasau · 2023-12-19T14:46:42Z

This PR changes how we write the Qiskit release-notes.html to correctly split the notes by version, updating the previous files if we find any change. If we already have a file for the version we updated, the script adds the new entries without overwriting the file and sorts the entries by patch. If the patch we modified was already in the version file, it substitutes its section for the new one.

Qiskit v0.45 has been regenerated using the new script to split correctly versions 0.45 and 0.25 to remove the last one from the file. Historical release notes files (< 0.45) will no longer be updated in the regeneration. For more information see: Qiskit/qiskit#11436

Command used:
npm run gen-api -- -p qiskit -v 0.45.0 -a https://github.com/Qiskit/qiskit/suites/17881600359/artifacts/1026798160

Closes #359

Eric-Arellano

Thanks for diving into this! I know it's pretty complex logic. You did a great job here abstracting it well and writing effective tests so we can be confident it works.

scripts/lib/releaseNotes.ts

Eric-Arellano · 2023-12-19T15:56:06Z

scripts/lib/releaseNotes.ts

+    if (!(await pathExists(versionPath))) {
+      // We don't have any release note file for that version
+      continue;
+    }


Hm, when would this happen? For example, if we're adding a brand new Qiskit version, then we should probably set up the new file?

This is an edge case possible if, for some reason, we generate two versions at the same time. For example, if we add a version using the historical argument, the release notes will not be created, and when we add the current version, the file for the former won't exist.

This shouldn't happen often, but if someday we have to regenerate several versions, for some reason, the script will handle the case correctly.

Oh - I think this probably speaks to my other comment about realizing I think we have a bug how --historical stops us from generating release notes?

I continue to be confused about this point.

we generate two versions at the same time.

That's not technically possible. The script only can generate one release at a time.

For example, if we add a version using the historical argument, the release notes will not be created, and when we add the current version, the file for the former won't exist.

I don't think I'm understanding. If the historical arg has 0.44 and 0.45, then we should expect both those release note files to exist. Now we generate current, which has 0.44, 0.45, and 0.46: it should generate all three of those files.

That's not technically possible.

Yes, sorry, I meant in the same PR. In a situation where you skipped one version, and you want now to add the current and the previous you skipped as historical without having it generated as current before.

I don't think I'm understanding.

The problem that I wanted to describe can't happen anymore because now, the script generates release notes even when we use the historical argument. However, the problem is still there.

When you generate a version for the first time, the release notes for that new version are not present in the release notes folder. The script will read the release notes (html converted to md) in order to find old versions we modified, and we need to update from the files in the folder. To update the old versions, we need to read the current release notes for those versions. The catch is that the script will try to read the current version as well, but the file doesn't exist yet, so we need to prevent it from breaking.

We need to read the current release notes files because we don't want to overwrite everything we had. There are some parts like the front matter or text added manually (like the table in qiskit v0.44 or a custom description) that we want to preserve. Your comment made me realize that we don't need to go through each section because the release notes should contain all the versions by default, so we can just overwrite that part with the new content. In the last commit, I removed some unnecessary code. Thank you Eric.

scripts/commands/updateApiDocs.ts

scripts/lib/releaseNotes.ts

Eric-Arellano

Looking good!

scripts/commands/updateApiDocs.ts

scripts/lib/releaseNotes.test.ts

scripts/lib/releaseNotes.ts

Eric-Arellano · 2023-12-20T17:55:14Z

scripts/lib/releaseNotes.ts

+    if (!(await pathExists(versionPath))) {
+      // We don't have any release note file for that version
+      continue;
+    }


Oh - I think this probably speaks to my other comment about realizing I think we have a bug how --historical stops us from generating release notes?

scripts/lib/releaseNotes.ts

Eric-Arellano

This overall looks great! I only have one doubt still. Thanks for working on this!

scripts/commands/updateApiDocs.ts

Eric-Arellano · 2023-12-28T17:33:32Z

scripts/lib/releaseNotes.ts

+    if (!(await pathExists(versionPath))) {
+      // We don't have any release note file for that version
+      continue;
+    }


I continue to be confused about this point.

we generate two versions at the same time.

That's not technically possible. The script only can generate one release at a time.

For example, if we add a version using the historical argument, the release notes will not be created, and when we add the current version, the file for the former won't exist.

I don't think I'm understanding. If the historical arg has 0.44 and 0.45, then we should expect both those release note files to exist. Now we generate current, which has 0.44, 0.45, and 0.46: it should generate all three of those files.

scripts/lib/releaseNotes.ts

Eric-Arellano · 2023-12-28T17:35:32Z

scripts/lib/sphinx/sphinxHtmlToMarkdown.ts

+
+      if (isReleaseNotes && releaseNotesTitle) {
+        // Release notes links should point to the current version
+        $img.attr("src", dest.replace(/[0-9].*\//, ""));


Interesting. What was happening before this change?

Before this change, if we used the historical argument, the release notes had all the links correct but the images. The images were pointing to the historical folder instead of the current version. For example, public/images/api/qiskit/0.44/circuit-1.png instead of public/images/api/qiskit/circuit-1.png

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>

scripts/lib/releaseNotes.ts

Part of #495. -- This PR adds two arguments to `npm run check:links`, `--current-apis` and `--qiskit-release-notes`. In a follow up, we'll add `--historical-apis`. By default, we don't do any of these checks since it's pretty rare for a PR author to be changing those files and we want `npm run check` to be as fast as possible locally. CI will still check the whole thing, though. This makes the script around 2x faster: ``` npm run check:links 4.04s user 0.32s system 156% cpu 2.797 total npm run check:links -- --current-apis --qiskit-release-notes 8.66s user 0.99s system 134% cpu 7.182 total ``` -- To fix the actual broken links, I manually fixed problems in release notes up to 0.44. We are unlikely to regenerate these release notes, so manual fixes should be fine. I did not fix 0.45, though, since we will continue to regenerate those release notes per #537. We will need to fix the underlying problem in the API docs, or in our API generation script.

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>

Part of Qiskit#495. -- This PR adds two arguments to `npm run check:links`, `--current-apis` and `--qiskit-release-notes`. In a follow up, we'll add `--historical-apis`. By default, we don't do any of these checks since it's pretty rare for a PR author to be changing those files and we want `npm run check` to be as fast as possible locally. CI will still check the whole thing, though. This makes the script around 2x faster: ``` npm run check:links 4.04s user 0.32s system 156% cpu 2.797 total npm run check:links -- --current-apis --qiskit-release-notes 8.66s user 0.99s system 134% cpu 7.182 total ``` -- To fix the actual broken links, I manually fixed problems in release notes up to 0.44. We are unlikely to regenerate these release notes, so manual fixes should be fine. I did not fix 0.45, though, since we will continue to regenerate those release notes per Qiskit#537. We will need to fix the underlying problem in the API docs, or in our API generation script.

This PR changes how we write the Qiskit `release-notes.html` to correctly split the notes by version, updating the previous files if we find any change. If we already have a file for the version we updated, the script adds the new entries without overwriting the file and sorts the entries by patch. If the patch we modified was already in the version file, it substitutes its section for the new one. Qiskit v0.45 has been regenerated using the new script to split correctly versions 0.45 and 0.25 to remove the last one from the file. Historical release notes files (< 0.45) will no longer be updated in the regeneration. For more information see: Qiskit/qiskit#11436 Command used: `npm run gen-api -- -p qiskit -v 0.45.0 -a https://github.com/Qiskit/qiskit/suites/17881600359/artifacts/1026798160` Closes Qiskit#359 --------- Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>

arnaucasau added 7 commits December 18, 2023 19:54

Handle qiskit release notes

fe5d757

use script to fix release notes

f147f9a

fix lint

7df5b0e

fix spacing

2b88f45

use regex for the versions

cb44adc

new sort function

9c257ef

minor changes

a8b080b

Eric-Arellano mentioned this pull request Dec 19, 2023

Remove Qiskit Terra 0.25 from release_notes.rst Qiskit/qiskit#11436

Merged

arnaucasau added 2 commits December 20, 2023 14:35

restore release notes

4abf3b5

fix order and remove qiskit terra 0.25

5f536f5

arnaucasau changed the title ~~[WIP] Handle Qiskit release notes structure correctly~~ Handle Qiskit release notes structure correctly Dec 20, 2023

change comment

35d0124

arnaucasau marked this pull request as ready for review December 20, 2023 15:50

Eric-Arellano reviewed Dec 20, 2023

View reviewed changes

arnaucasau added 3 commits December 20, 2023 18:19

incorporate feedback

f5246f8

TODO comment

ae5db03

fix bug

1bf930d

Eric-Arellano reviewed Dec 20, 2023

View reviewed changes

arnaucasau added 2 commits December 20, 2023 18:56

fileToInitialHeaders variable

c40873c

feedback & allow historical write release notes

a233815

Eric-Arellano mentioned this pull request Dec 21, 2023

Release note images should always be in the current folder #544

Closed

update comment

5e6600d

Eric-Arellano reviewed Dec 27, 2023

View reviewed changes

scripts/lib/releaseNotes.ts Outdated Show resolved Hide resolved

arnaucasau added 2 commits December 28, 2023 16:14

remove todo

51e0baf

fix test

06e6590

Eric-Arellano reviewed Dec 28, 2023

View reviewed changes

Eric-Arellano mentioned this pull request Dec 28, 2023

Check links for release notes #559

Merged

arnaucasau and others added 2 commits December 28, 2023 19:17

Update scripts/lib/releaseNotes.ts

15416ae

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>

remove unnecessary code

7aa586f

Eric-Arellano reviewed Dec 29, 2023

View reviewed changes

scripts/lib/releaseNotes.ts Outdated Show resolved Hide resolved

Eric-Arellano approved these changes Dec 29, 2023

View reviewed changes

Incorporate feedback

75c4d7d

Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>

arnaucasau added this pull request to the merge queue Dec 29, 2023

Merged via the queue into Qiskit:main with commit deb9898 Dec 29, 2023

arnaucasau deleted the qiskit-release-notes branch December 29, 2023 16:07

Handle Qiskit release notes structure correctly #537

Handle Qiskit release notes structure correctly #537

Uh oh!

Conversation

arnaucasau commented Dec 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Eric-Arellano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Eric-Arellano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Eric-Arellano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

arnaucasau commented Dec 19, 2023 •

edited

Loading