napari / docs

Documentation for napari (other than API docs)
BSD 3-Clause "New" or "Revised" License
9 stars 37 forks source link

Unify docs workflows #348

Closed Czaki closed 3 months ago

Czaki commented 5 months ago

Description

Closes: https://github.com/napari/docs/issues/284

This PR:

Unify build workflows from two to one.

Remove .doctree directory (save 400MB)

psobolewskiPhD commented 4 months ago

Should delete the .doctrees from the uploaded artifact IMO. They aren't needed for anything related to the html as far as I understand.

Czaki commented 4 months ago

I do not know. Maybe they are usefull for some debug. I'm not such familiar with sphinx

psobolewskiPhD commented 4 months ago

here's the info:

Since Sphinx has to read and parse all source files before it can write an output file, the parsed source files are cached as “doctree pickles”. Normally, these files are put in a directory called .doctrees under the build directory; with this option you can select a different cache directory (the doctrees can be shared between all builders).

https://www.sphinx-doc.org/en/master/man/sphinx-build.html#cmdoption-sphinx-build-d

They aren't needed for the website. I guess instead of delete, we could change the dir to not put them in the folder used for the artifact?

jni commented 4 months ago

And I agree with @psobolewskiPhD that maybe we should just configure the .doctree dir to be in /tmp or something. And/or make a PR to sphinx to not put it in the build in the first place. 😂

jni commented 4 months ago

@Czaki

maybe rename the workflow to build_and_deploy.yml?

maybe we should just configure the .doctree dir to be in /tmp or something.

Do you want to implement either or both of these, or none?

psobolewskiPhD commented 4 months ago

I did some more digging around the sphinx docs, readthedocs docs, etc. The defaults these days is to use -M instead of -b which puts the doctrees separate from the html. See: https://www.sphinx-doc.org/en/master/man/sphinx-build.html#cmdoption-sphinx-build-M (html is the default builder) So the you'd get:

docs/_build/html
docs/_build/doctrees

See for example: https://www.sphinx-doc.org/en/master/tutorial/deploying.html#id5 where only the html is deployed to Github Pages.

psobolewskiPhD commented 4 months ago

I second the motion to change the name to build_and_deploy I made a PR suggesting what I think the the proper way to handle the doctrees question.

Looking at the circleCI output of that PR, the artifact is ~126MB https://app.circleci.com/pipelines/github/psobolewskiPhD/napari-docs/4/workflows/8173ddf0-6858-4024-af40-38e2abd91cb8/jobs/4/steps?invite=true#step-107-138189_28

While the circleCI output of this current branch has the artifact ~700 Mb: https://app.circleci.com/pipelines/github/Czaki/napari-docs/73/workflows/a3651b6d-4eab-4eec-b947-b875e80696c8/jobs/73?invite=true#step-107-169391_28

jni commented 4 months ago

I made a PR suggesting what I think the the proper way to handle the doctrees question.

Fantastic detective work @psobolewskiPhD! 🕵️ 🤓

psobolewskiPhD commented 4 months ago

Per docs here https://github.com/larsoner/circleci-artifacts-redirector-action The circleCI redirector is expected to not work properly until this is merged.

Czaki commented 4 months ago

I think everything is good now. @Czaki Can you sanity check the tags bit?

What tags?

psobolewskiPhD commented 4 months ago

This fix: https://github.com/napari/docs/pull/348/commits/928551d5144ba182a629181ea96c1b4c9a94d185 to this error: https://github.com/napari/docs/actions/runs/8103613500

Czaki commented 4 months ago

This fix: 928551d to this error: napari/docs/actions/runs/8103613500

You suggest change in getting value from tags and point skipped action (caused by file rename I think).

psobolewskiPhD commented 4 months ago

Sorry if I wasn't being clear. This action was failing with:

The workflow is not valid. .github/workflows/build_and_deploy.yml (Line: 92, Col: 14): Unexpected symbol: 'ref/refs\/tags\//'. Located at position 8 within expression: github.ref/refs\/tags\//

Here's that code section before my fix: https://github.com/napari/docs/blob/8b695ca58ebda1d186f8feba9c5bea5fd47d304a/.github/workflows/build_and_deploy.yml#L92-L97

As far As I can tell, in github.ref/refs\/tags\// /refs\/tags\// is trying to get the name of the tag, so my fix is to use it directly, based on https://docs.github.com/en/actions/learn-github-actions/contexts

psobolewskiPhD commented 4 months ago

Because this is for deployment, it's not easy to check, so hence asking you to sanity check before we merge this and find out docs don't deploy right!

melissawm commented 3 months ago

It looks like this is good to go, pending @psobolewskiPhD 's comment above? If so, I just want to point out that if the deploy fails it's an easy fix and doesn't break the website (it will only not show the latest version until we fix it). So I'd say this is pretty safe.

psobolewskiPhD commented 3 months ago

I pushed the changes to the workflow from the video conversion PR.

melissawm commented 3 months ago

I am marking this as ready as I believe it's done - @Czaki last chance to take a look 😄

Czaki commented 3 months ago

I think it is ready. Still do not know what with deploy trigger.