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

Unify docs workflows #348

Merged
merged 15 commits into from
Mar 27, 2024
Merged

Unify docs workflows #348

merged 15 commits into from
Mar 27, 2024

Conversation

Czaki
Copy link
Contributor

@Czaki Czaki commented Feb 1, 2024

Description

Closes: #284

This PR:

Unify build workflows from two to one.

Remove .doctree directory (save 400MB)

@Czaki Czaki added the maintenance CI, dependencies, and other maintenance label Feb 1, 2024
@Czaki Czaki added this to the 0.5.0 milestone Feb 1, 2024
@Czaki Czaki requested a review from melissawm February 1, 2024 11:16
@github-actions github-actions bot added the task label Feb 1, 2024
Copy link
Member

@melissawm melissawm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

@psobolewskiPhD
Copy link
Member

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
Copy link
Contributor Author

Czaki commented Feb 15, 2024

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

@psobolewskiPhD
Copy link
Member

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?

Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe rename the workflow to build_and_deploy.yml?

@jni
Copy link
Member

jni commented Feb 20, 2024

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
Copy link
Member

jni commented Feb 27, 2024

@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
Copy link
Member

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
Copy link
Member

psobolewskiPhD commented Feb 28, 2024

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

Copy link
Member

@psobolewskiPhD psobolewskiPhD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I second the motion to rename the new workflow to build_and_deploy.yml

@jni
Copy link
Member

jni commented Feb 29, 2024

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

Fantastic detective work @psobolewskiPhD! 🕵️ 🤓

.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
@psobolewskiPhD
Copy link
Member

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

Copy link
Member

@psobolewskiPhD psobolewskiPhD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Czaki
Copy link
Contributor Author

Czaki commented Mar 2, 2024

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

What tags?

@psobolewskiPhD
Copy link
Member

This fix:
928551d
to this error:
https://github.com/napari/docs/actions/runs/8103613500

@Czaki
Copy link
Contributor Author

Czaki commented Mar 2, 2024

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
Copy link
Member

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:

run: |
if [[ "${{ github.ref }}" == "refs/tags/"* ]]; then
echo "branch_name=${{ github.ref/refs\/tags\// }}" >> $GITHUB_ENV
else
echo "branch_name=dev" >> $GITHUB_ENV
fi

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

@melissawm
Copy link
Member

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

@Czaki
Copy link
Contributor Author

Czaki commented Mar 25, 2024

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

@psobolewskiPhD psobolewskiPhD merged commit a77d2eb into napari:main Mar 27, 2024
7 checks passed
@Czaki Czaki deleted the fix_docs_build branch March 27, 2024 20:41
psobolewskiPhD added a commit to napari/napari that referenced this pull request Apr 5, 2024
# References and relevant issues
After merged, docs deployment is failing, see:
https://github.com/napari/napari/actions/runs/8569580333/job/23485774671
This is due to the napari/docs workflow change:
napari/docs#348

# Description
In this PR I change the name of the triggered workflow to the correct
new name.
psobolewskiPhD added a commit that referenced this pull request Apr 16, 2024
* Update `docs_deployment.md` now that we unified workflows in #348.
* Change workflow name to be Build & Deploy PR Docs, since #348
* Add file paths to the workflow to aid other working on it

---------

Co-authored-by: Peter Sobolewski <[email protected]>
psobolewskiPhD added a commit to napari/napari that referenced this pull request Apr 19, 2024
#6814)

# References and relevant issues
I noticed CircleCI docs redirector is not working since
napari/docs#348
See, e.g.
https://output.circle-artifacts.com/output/job/f7b7b3bf-0e97-4ca0-a6dd-2f30998c625d/artifacts/0/docs/docs/_build/index.html?pr=6807

The docs build correctly, but the structure is different because of the
change to the Makefile to put the html in `/html`

# Description
Updates config.yml to match the one in napari/docs
https://github.com/napari/docs/blob/main/.circleci/config.yml
and also fixes the redirector workflow to point to the right location.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance CI, dependencies, and other maintenance task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use same workflow for build_docs and build-and-deploy
4 participants