build: Add snapshot generator #1183

mofojed · 2025-05-08T21:33:39Z

Run npm run update-doc-snapshots to extract all test-sets from docs, and then run them against the latest deephaven-core
- Created a custom directive to pass through the info string on code blocks, because the default myst parser just drops them
- Snapshots are outputted to docs/snapshots, then copied into the build/markdown folder when the docs are built
- This PR does not include those snapshots yet, will have another PR just to check in those snapshots
- Plotly express snapshots require some changes in Salmon to add a PlotlyExpressSerializer (I have this in another branch)
Tested with the update Salmon with a plotly express preview component: https://github.com/deephaven/salmon/pull/41

- Run `npm run update-doc-snapshots` to extract all test-sets from docs, and then run them against the latest deephaven-core - Created a custom directive to pass through the info string on code blocks, because the default myst parser just drops them - Snapshots are outputted to docs/snapshots, then copied into the build/markdown folder when the docs are built - This PR does not include those snapshots yet, will have another PR just to check in those snapshots - Plotly express snapshots require some changes in Salmon to add a PlotlyExpressSerializer (I have this in another branch)

- It doesn't get parsed by Myst correctly, and outputs garbage - Looks like Myst can't handle nested code blocks

docker-compose.docs-snapshots.yml

mattrunyon · 2025-05-13T02:25:54Z

docker-compose.docs-snapshots.yml

+    volumes:
+      - ./plugins/ui/docs/build/markdown:/extract/ui
+      - ./plugins/plotly-express/docs/build/markdown:/extract/plotly-express
+      - ./docker/build/test-sets:/results # Output to a temporary build directory


I think you could specify this as a docker volume at the top level of the compose file instead and then it won't need an intermediary directory on the host. The potential downside is less visibility into junk in this directory if it's not cleared by the extractor before running. Docker will create and manage the volume instead of using the host as a middleman.

volumes: test-sets: services: extractor: volumes: - test-sets:/results

You can also still use the root volume definition to create a bind mount if you wanted it visible in the repo files. I think it works like this for the volume definition and then same to attach it to the service. The benefit of this would just be there's no accidental typo in the definition between the 2 containers, but that doesn't really matter in this case since it's only used twice

volumes: test-sets: driver: local driver_opts: o: bind device: ./docker/build/test-sets

Interestingly Copilot suggested exactly this, but with type: none added to driver_opts. Docs from driver_opts show type being specified: https://docs.docker.com/reference/compose-file/volumes/#driver_opts
I'll try with omitting it though.

So doing it this way throws an error if the directory doesn't already exist:

✔ deephaven-plugins-docs-validator Pulled 1.0s Error response from daemon: error while mounting volume '/var/lib/docker/volumes/deephaven-plugins_validator-results/_data': failed to mount local volume: mount /home/bender/dev/deephaven/iris-oss/deephaven-plugins/docker/build/validator-results:/var/lib/docker/volumes/deephaven-plugins_validator-results/_data, flags: 0x1000: no such file or directory ✔ deephaven-plugins-docs-validator Pulled 1.0s Error response from daemon: error while mounting volume '/var/lib/docker/volumes/deephaven-plugins_validator-results/_data': failed to mount local volume: mount /home/bender/dev/deephaven/iris-oss/deephaven-plugins/docker/build/validator-results:/var/lib/docker/volumes/deephaven-plugins_validator-results/_data, flags: 0x1000: no such file or directory

Not sure how to get around that without creating the directories first (outside of the docker compose file). It sounds like the path must exist for it to work: https://forums.docker.com/t/can-named-volume-in-docker-compose-file-create-a-folder-if-it-does-not-exist/143625

I think I'll just switch this back? Unless we want to git check-in those folders then ignore the contents...

Doing it which way doesn't work? The bind mount as a named volume?

I'm fine without it, just thought it was worth trying. Only 2 locations and it would throw or not work if they didn't match.

Or the non-bind named volume where Docker handles it and it doesn't create the intermediate test-set files in the repo. Unless you want easier inspection of the test-set files

The bind mount as a named volume doesn't work if the directory doesn't already exist.

docker-compose.docs-snapshots.yml

plugins/plotly-express/docs/bar.md

plugins/ui/docs/components/markdown.md

sphinx_ext/make_docs_utilities.py

tools/run_docker.sh

mattrunyon

Looks good (assuming there wasn't an issue with the named volume bind mount)

tests/docker-compose.yml

- Added snapshots for plotly-express/deephaven.ui - Cleaned up issues in docs that were causing issues with snapshots - Wired up a `--snapshots` action for taking snapshots of docs

github-actions · 2025-05-15T13:59:57Z

plotly-express docs preview (Available for 14 days)

github-actions · 2025-05-15T13:59:59Z

ui docs preview (Available for 14 days)

mofojed · 2025-05-15T19:39:30Z

docker-compose.docs-snapshots.yml

+      ]
+    volumes:
+      - ./plugins/ui/docs/build/markdown:/extract/ui
+      - ./plugins/plotly-express/docs/build/markdown:/extract/plotly-express


@mattrunyon this is something that's kind of weird. So we're extracting the snapshots/generating them based on the built docs - main reason for this is if I include the source docs directory, then everything gets extracted twice (since the build directory is a descendent of the source directory). The other reason is to also verify that the generated docs actually have the correct order/test-set etc parameters set after generating.

Downside of this is the snapshots are in the docs source directory, and then need to be copied into the build directory for deploying to Salmon (or locally). So you need to 1. Build docs 2. Run snapshots 3. Copy snapshots into the build directory (we don't put them in there directly since we want them to persist).

Wondering if I should just test on the source directory instead? Would need to exclude the build directory, and we wouldn't catch issues with the code blocks potentially getting mutilated in the build step... any other ideas?

Should be fine to run on source. The salmon validator on PRs will check if the build directory is invalid including snapshot issues.

The snapshot generator should only generate snapshots for those that don't exist, right? So most times you run it, it shouldn't actually create any files? Unless you add/modify code blocks.

Okay I've got it running on source now, and then copying over snapshots as part of the build.

- Will be validated in build directory - Copy snapshots as part of the build step

…shots

github-actions · 2025-05-20T15:37:30Z

plotly-express docs preview (Available for 14 days)

- Some orders not defined correctly

github-actions · 2025-05-20T20:27:03Z

plotly-express docs preview (Available for 14 days)

github-actions · 2025-05-20T20:27:18Z

ui docs preview (Available for 14 days)

…shots

github-actions · 2025-05-20T20:42:06Z

ui docs preview (Available for 14 days)

github-actions · 2025-05-20T20:54:25Z

plotly-express docs preview (Available for 14 days)

github-actions · 2025-05-20T20:54:30Z

ui docs preview (Available for 14 days)

mattrunyon

The .dockerignore file needs to add **/docs and **/.tox. The cache is busting after generating new snapshots and causing a 2 min rebuild

mattrunyon · 2025-05-20T21:06:03Z

README.md

+You can also re-generate the snapshots for the docs by passing the `--snapshots` flag. This should be done when new code blocks are added to the docs.
+
+This example will build the docs for the `ui` plugin and re-generate the snapshots:
+
+```shell
+python tools/plugin_builder.py --docs --snapshots ui
+```


Should there be a section about running this with npm as well? It looks like the flag just runs the npm script under the hood. Should we even make it a separate flag or just bake it into the --docs flag?

The annoying thing is how long the docker snapshot image takes to build. Takes me like 2.5-3 min. So I could see that being an argument for not running it with the docs flag

Yea I specifically had it as a separate flag as the snapshots container takes much longer to load if you haven't already built it.
I'll add a note about updating snapshots using npm...

- Now it doesn't rebuild everything when just updating the docs

github-actions · 2025-05-21T14:37:06Z

ui docs preview (Available for 14 days)

github-actions · 2025-05-21T14:37:11Z

plotly-express docs preview (Available for 14 days)

github-actions · 2025-05-21T15:23:47Z

plotly-express docs preview (Available for 14 days)

github-actions · 2025-05-21T15:23:51Z

ui docs preview (Available for 14 days)

mofojed requested a review from mattrunyon May 8, 2025 21:33

mofojed self-assigned this May 8, 2025

github-actions bot requested a review from margaretkennedy May 8, 2025 21:33

mofojed added 5 commits May 12, 2025 15:05

A few more fixes from running snapshots

cb18f23

Fix another deephaven.ui doc

193ab68

Fix the path to the snapshots

c1a7e41

Disable the markdowns Code Blocks example

2a16a29

- It doesn't get parsed by Myst correctly, and outputs garbage - Looks like Myst can't handle nested code blocks

Add all the snapshots

aa99279

mattrunyon requested changes May 13, 2025

View reviewed changes

tools/run_docker.sh Show resolved Hide resolved

Clean up based on review comments

4a8b4c7

mofojed requested a review from mattrunyon May 13, 2025 13:03

Add a script to validate docs

0cc1597

mattrunyon previously approved these changes May 13, 2025

View reviewed changes

mattrunyon reviewed May 13, 2025

View reviewed changes

tests/docker-compose.yml Show resolved Hide resolved

margaretkennedy previously approved these changes May 14, 2025

View reviewed changes

Fix up the docker compose context

3f259e3

mofojed dismissed stale reviews from margaretkennedy and mattrunyon via 3f259e3 May 14, 2025 15:20

mofojed requested a review from mattrunyon May 14, 2025 15:22

Update snapshots

d3b6bdb

- Added snapshots for plotly-express/deephaven.ui - Cleaned up issues in docs that were causing issues with snapshots - Wired up a `--snapshots` action for taking snapshots of docs

mofojed commented May 15, 2025

View reviewed changes

mofojed added 5 commits May 16, 2025 08:47

Take snapshots from source

9cded33

- Will be validated in build directory - Copy snapshots as part of the build step

Remove unused snapshots

e3513d2

Merge remote-tracking branch 'origin/main' into DOC-551-doc-with-snap…

8c155a7

…shots

Merge remote-tracking branch 'origin/main' into DOC-551-doc-with-snap…

55d0862

…shots

New plotly express snapshots

f60dca1

Fix broken snapshots

2421584

- Some orders not defined correctly

Merge remote-tracking branch 'origin/main' into DOC-551-doc-with-snap…

489a906

…shots

Update titles and legends plots snapshots

69fbd27

mattrunyon reviewed May 20, 2025

View reviewed changes

mofojed added 2 commits May 21, 2025 10:33

Update dockerignore

961133c

- Now it doesn't rebuild everything when just updating the docs

Update README with update-doc-snapshots instructions

122446b

mofojed requested a review from mattrunyon May 21, 2025 15:06

mattrunyon approved these changes May 21, 2025

View reviewed changes

mofojed merged commit 5b5ee61 into deephaven:main May 21, 2025
25 checks passed

build: Add snapshot generator #1183

build: Add snapshot generator #1183

Uh oh!

Conversation

mofojed commented May 8, 2025

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!

mattrunyon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented May 15, 2025

Uh oh!

github-actions bot commented May 15, 2025

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!

github-actions bot commented May 20, 2025

Uh oh!

github-actions bot commented May 20, 2025

Uh oh!

github-actions bot commented May 20, 2025

Uh oh!

github-actions bot commented May 20, 2025

Uh oh!

github-actions bot commented May 20, 2025

Uh oh!

github-actions bot commented May 20, 2025

Uh oh!

mattrunyon left a comment

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!

github-actions bot commented May 21, 2025

Uh oh!

github-actions bot commented May 21, 2025

Uh oh!

github-actions bot commented May 21, 2025

Uh oh!

github-actions bot commented May 21, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone