Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deal with large and inaccessible Qiskit GIF #2533

Open
Eric-Arellano opened this issue Dec 26, 2024 · 6 comments · Fixed by Qiskit/qiskit#13616
Open

Deal with large and inaccessible Qiskit GIF #2533

Eric-Arellano opened this issue Dec 26, 2024 · 6 comments · Fixed by Qiskit/qiskit#13616
Assignees

Comments

@Eric-Arellano
Copy link
Collaborator

Eric-Arellano commented Dec 26, 2024

See https://github.com/Qiskit/documentation/blob/main/public/images/api/qiskit/depth.gif and its usage:

A particularly important circuit property is known as the circuit [`depth()`](#qiskit.circuit.QuantumCircuit.depth "qiskit.circuit.QuantumCircuit.depth"). The depth of a quantum circuit is a measure of how many “layers” of quantum gates, executed in parallel, it takes to complete the computation defined by the circuit. Because quantum gates take time to implement, the depth of a circuit roughly corresponds to the amount of time it takes the quantum computer to execute the circuit. Thus, the depth of a circuit is one important quantity used to measure if a quantum circuit can be run on a device.
The depth of a quantum circuit has a mathematical definition as the longest path in a directed acyclic graph (DAG). However, such a definition is a bit hard to grasp, even for experts. Fortunately, the depth of a circuit can be easily understood by anyone familiar with playing [Tetris](https://en.wikipedia.org/wiki/Tetris). Lets see how to compute this graphically:
![../\_images/depth.gif](/images/api/qiskit/depth.gif)
We can verify our graphical result using [`QuantumCircuit.depth()`](#qiskit.circuit.QuantumCircuit.depth "qiskit.circuit.QuantumCircuit.depth"):
```python
assert qc.depth() == 9
```

Issues with GIFs:

  1. GIFs violate accessibility because of flashing automatically and not having a way to pause them
  2. The GIF is 2.7mb, which makes the page slower to download for users, even with the lazy loading Next.js uses. It also bloats our Git repository size in qiskit/documentation

Ideas for workarounds:

  1. Replace it with a link to our 1 minute video "What is circuit depth": https://www.youtube.com/watch?v=7AVIc7SkX3M
  2. Use text-based only explanation
  3. Remove it entirely

Migrating the file from GIF to AVIF only reduced the image from 2.7MB to 2.3MB, and it corrupted the GIF.

Note that this GIF is in our historical API docs. We should probably rewrite them to not have the image so that those pages load quicker & our repo size is smaller.

--

Response from Jake L in Slack, Dec 27

I tried to kill it once (for one, I disagree that it’s anything to do with tetris!), but there was some sentiment from within the qiskit-compiler team that it had helped with the understanding.

Fwiw, I feel like the entire “simple circuit metrics” section could do with a rework. It’s generally seemed to me to be targeted at a (now 6+ years ago) QC theorist who doesn’t know how to program, which probably was a core demographic at the time when Qiskit was in its infancy, but it’s not the best pedagogy now. (Fwiw, that section was mostly unaltered when the rest of the QuantumCircuit page was rewritten, because it was already good enough compared to the complete absence of everything else.)

That is to say, I’m strongly in favour of removing the GIF, and I feel like the description of “circuit depth” should have a text overview in the QuantumCircuit.depth and DAGCircuit.depth docstrings. If people think it merits a particular further explanation, the youtube link is fine to include for me, though equally that content to me is a bit more about the theory of QC and doesn’t necessarily belong in the API docs.

@Eric-Arellano Eric-Arellano changed the title Deal with large Qiskit GIF Deal with large and inaccessible Qiskit GIF Dec 26, 2024
@Eric-Arellano
Copy link
Collaborator Author

@abbycross and I discussed that we should remove this explanatory content, as Jake suggested. That is acceptable given the current strategy of the docs that we are not focused on introductory explanation of theory anymore.

Once Abby applies that change, the infra team will help backport the change to historical API docs to fix all historical violations.

@jakelishman
Copy link
Member

We should probably rewrite them to not have the image so that those pages load quicker & our repo size is smaller.

Fwiw, removing the GIF from the pages won't change the repo size. It'll change the size of a checkout on disk (which I suppose is still useful), but not the size needed to clone - the blob's already been included in the history, so removing it won't change things now. If repo size is a concern, the biggest thing would probably be to not have checked rendered / generated pages into this repo - at the moment, you're paying the weight of 20-odd complete current renders, plus every single historical render that's every been checked in, and checking out a new ref means traversing a whole load of that history rewriting those files.

The GIF itself has never been changed since its inclusion, so it's 2.7MB of dead weight, but it's just a single blob - it's only stored once in the repo data (though a checkout will make many temporary copies of that blob, but that's disk-bound, not network-bound work).

@Eric-Arellano
Copy link
Collaborator Author

Eric-Arellano commented Jan 6, 2025

Fwiw, removing the GIF from the pages won't change the repo size. It'll change the size of a checkout on disk (which I suppose is still useful), but not the size needed to clone - the blob's already been included in the history, so removing it won't change things now.

Ack. Although #2539 & shallow clones mean removing this will help with initial clones.

but it's just a single blob

Yes, but a large blob duplicated on every version. #2525 has an idea to de-duplicate duplicate images, although it's set to low priority.

--

All that being said, the biggest motivation is accessibility and GA compliance.

@jakelishman
Copy link
Member

a large blob duplicated on every version

It's not duplicated in the .git repo itself, just in the checkout of a repo 1. Since that's disk bound IO and it's just one file with no history, I'd wager it's a lot less of your dev-machine's time than calculating and repacking all the histories of all the generated repo files - git will spend a lot more time churning through deltas on the generated files that are modified, and you'll have more filesystem overhead from dealing with those.

If the duplication on disk of a checkout is a problem, then the root cause is likely. more that all the generated files for each build are stored in the "human working" repo, rather than one specific file. Would something like a git submodule-based system for the generated files work, so the main human repo includes only a commit ref per package per minor version, and git's instructed to shallow clone those refs? You'd still have the history of "when we regenerated the files" in the human repo, but the actual generated files would be somewhere else entirely, and the developer docs build could be configured to not require the submodules to have been fetched.

Footnotes

  1. try something like git ls-tree -rl HEAD | sort -k4 -n, and the depth.gif name will be part of many file paths that all refer to the same "blob".

@jakelishman
Copy link
Member

Of course, my system would still need an entire history re-write to remove the existing cost of the initial clone, since those files have already been committed.

And yeah, I'm still 100% in favour of getting rid of the GIF from the docs. Mostly just commenting on the points about its effect on the git experience.

@Eric-Arellano
Copy link
Collaborator Author

We need to apply Qiskit/qiskit#13616 to all historical versions of Qiskit, as well. We do that by modifying the Box artifact if the version is no longer actively maintained, else backporting it in the Qiskit repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

3 participants