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

Containerize documentation build environment and add sphinx-autobuild for hot-reload #4960

Merged

Conversation

MortalHappiness
Copy link
Member

@MortalHappiness MortalHappiness commented Feb 27, 2024

Tracking issue

N/A

Why are the changes needed?

The documentation build environment is complex and buggy for MacOS ARM users.

What changes were proposed in this pull request?

Added one command: make dev-docs to use docker container to build the documentation environement.

Supported Environment variables:

  • DEV_DOCS_WATCH: If set, the docs will be built and served using sphinx-autobuild for live updates.
  • FLYTEKIT_LOCAL_PATH: If set, the local path to flytekit will be used instead of the source code from the flyteorg/flytekit repo.
  • FLYTECTL_LOCAL_PATH: If set, the local path to flytectl will be used instead of the source code from the flyteorg/flytectl repo.
  • FLYTESNACKS_LOCAL_PATH: If set, the local path to flytesnacks will be used instead of the source code from the flyteorg/flytesnacks repo

How was this patch tested?

Setup process

First you need to make sure you can run linux/amd64 container

You can reference this article to setup the environment for MacOS arm CPU.

Steps:

  • make dev-docs
  • python -m http.server --directory docs/_build/html
  • And then go to http://localhost:8000 to see the documentation.

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. documentation Improvements or additions to documentation enhancement New feature or request labels Feb 27, 2024
@MortalHappiness MortalHappiness force-pushed the feature/containerize-doc-build-env branch 2 times, most recently from 43f5992 to 488166f Compare February 29, 2024 13:41
@MortalHappiness MortalHappiness marked this pull request as draft March 3, 2024 08:57
Copy link

codecov bot commented Mar 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.08%. Comparing base (7287470) to head (2d98234).
Report is 9 commits behind head on master.

❗ Current head 2d98234 differs from pull request most recent head f6e77ad. Consider uploading reports for the commit f6e77ad to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4960      +/-   ##
==========================================
+ Coverage   58.60%   59.08%   +0.48%     
==========================================
  Files         568      645      +77     
  Lines       51121    55651    +4530     
==========================================
+ Hits        29958    32880    +2922     
- Misses      18748    20174    +1426     
- Partials     2415     2597     +182     
Flag Coverage Δ
unittests 59.38% <ø> (+0.77%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MortalHappiness MortalHappiness force-pushed the feature/containerize-doc-build-env branch from 488166f to dcbd4bf Compare March 4, 2024 12:48
@MortalHappiness MortalHappiness marked this pull request as ready for review March 4, 2024 12:48
@MortalHappiness MortalHappiness marked this pull request as draft March 4, 2024 14:37
@MortalHappiness MortalHappiness marked this pull request as ready for review March 4, 2024 17:08
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Mar 4, 2024
@MortalHappiness MortalHappiness force-pushed the feature/containerize-doc-build-env branch 6 times, most recently from ef9b75b to 33ecd2f Compare March 5, 2024 03:05
@MortalHappiness MortalHappiness marked this pull request as draft March 5, 2024 16:54
@MortalHappiness MortalHappiness force-pushed the feature/containerize-doc-build-env branch from d50ad78 to a04ad1b Compare March 6, 2024 18:28
@MortalHappiness MortalHappiness marked this pull request as ready for review March 6, 2024 18:38
@jasonlai1218
Copy link
Contributor

jasonlai1218 commented Mar 10, 2024

My laptop is Mac M1 Pro. It can work.
run:

make dev-docs

docker buildx build --load --platform=linux/amd64 --build-arg USER_UID=$(id -u) --build-arg USER_GID=$(id -g) -t flyte-dev-docs:latest -f Dockerfile.docs .
[+] Building 2857.5s (15/15) FINISHED docker:desktop-linux
=> [internal] load build definition from Dockerfile.docs 0.0s
=> => transferring dockerfile: 795B 0.0s
=> [internal] load metadata for docker.io/condaforge/mambaforge:latest 4.7s
=> [auth] condaforge/mambaforge:pull token for registry-1.docker.io 0.0s
=> [internal] load .dockerignore 0.0s
=> => transferring context: 115B 0.0s
=> [stage-0 1/9] FROM docker.io/condaforge/mambaforge:latest@sha256:952d513ea7d37823fd6c53f706303847b9 97.4s
=> => resolve docker.io/condaforge/mambaforge:latest@sha256:952d513ea7d37823fd6c53f706303847b9d7fdd1a94 0.0s
=> => sha256:17d0386c2fff30a5b92652bbef2b84639dba9b9f17bdbb819c8d10badd827fdb 27.51MB / 27.51MB 41.9s
=> => sha256:ad01f4833820334510b8aeab83b58c5ecf2b1bd99850770a8c6521b4ba98dc13 118.81MB / 118.81MB 93.3s
=> => sha256:952d513ea7d37823fd6c53f706303847b9d7fdd1a94230c50243381fe234818a 2.36kB / 2.36kB 0.0s
=> => sha256:3f6ef750513c2b4c4db587bbcd5964e797ac5a81e29ab13f76d4cc0022bcb2ce 676B / 676B 0.0s
=> => sha256:d3b172a1616c13b806a124ae1aa6691d4f1979af7a8e5f1a24d5a621fa23ecfd 3.88kB / 3.88kB 0.0s
=> => extracting sha256:17d0386c2fff30a5b92652bbef2b84639dba9b9f17bdbb819c8d10badd827fdb 1.0s
=> => extracting sha256:ad01f4833820334510b8aeab83b58c5ecf2b1bd99850770a8c6521b4ba98dc13 3.9s
=> [internal] load build context 0.3s
=> => transferring context: 12.48MB 0.3s
=> [stage-0 2/9] RUN getent group "20" || groupadd --gid "20" flyte 0.6s
=> [stage-0 3/9] RUN getent passwd "502" || useradd --uid "502" --gid "20" -m flyte 0.3s
=> [stage-0 4/9] RUN conda install -c conda-forge conda-lock 90.9s
=> [stage-0 5/9] RUN --mount=type=bind,source=monodocs-environment.lock.yaml,target=monodocs-environ 2503.6s
=> [stage-0 6/9] COPY flyteidl flyteidl 0.3s
=> [stage-0 7/9] RUN python -m pip install sphinx-autobuild 6.1s
=> [stage-0 8/9] RUN python -m pip install ./flyteidl 17.5s
=> [stage-0 9/9] WORKDIR docs 0.0s
=> exporting to image 136.1s
=> => exporting layers 136.0s
=> => writing image sha256:b2eea68bcab38a8bd3aa38ebfaf73f8cd7f3239da36ae0602b5e6e4b04011e7e 0.0s
=> => naming to docker.io/library/flyte-dev-docs:latest 0.0s
touch .tmp_build/dev-docs-image
bash script/local_build_docs.sh
docker run --platform linux/amd64 --rm --pull never -v ./docs:/docs flyte-dev-docs:latest sphinx-build -M html . _build
Running Sphinx v4.5.0
loading translations [en]... done
making output directory... done
...
...
...
copying static files... done
copying extra files... done
dumping search index in English (code: en)... done
dumping object inventory... done
build succeeded, 4738 warnings.

The HTML pages are in _build/html.

and run

python -m http.server --directory docs/_build/html

Serving HTTP on :: port 8000 (http://[::]:8000/) ...
::1 - - [10/Mar/2024 17:18:56] "GET / HTTP/1.1" 200 -

Screenshot 2024-03-10 at 5 19 41 PM

@@ -77,4 +77,3 @@ dependencies:

platforms:
- linux-64
- osx-arm64
Copy link
Member

Choose a reason for hiding this comment

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

Why do we remove this?

* Install `conda-lock <https://github.com/conda/conda-lock>`__.
* In the ``flyteorg/flyte`` root directory run:
* ``conda-lock install --name monodocs-env monodocs-environment.lock.yaml``
* ``conda activate monodocs-env``
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep this so some people can still use conda to install if they don't want to run Docker locally?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in the latest commit.

@MortalHappiness MortalHappiness force-pushed the feature/containerize-doc-build-env branch from a04ad1b to 679cf00 Compare March 15, 2024 02:21
@neverett
Copy link
Contributor

@MortalHappiness thank you for all your work on this -- we're about to start a major docs reorganization project that will affect the docs build process, so let's hold off on merging this for now and come back to it in a few weeks when those changes are in place.

@MortalHappiness MortalHappiness force-pushed the feature/containerize-doc-build-env branch 2 times, most recently from 2d98234 to 00ea6fe Compare April 3, 2024 05:20
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Apr 3, 2024
pingsutw
pingsutw previously approved these changes Apr 12, 2024
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 12, 2024
@MortalHappiness MortalHappiness force-pushed the feature/containerize-doc-build-env branch from 00ea6fe to 7a5533b Compare April 12, 2024 05:25
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Apr 12, 2024
@MortalHappiness MortalHappiness force-pushed the feature/containerize-doc-build-env branch 2 times, most recently from be06ab5 to 5cc32c1 Compare April 17, 2024 14:22
docs/Dockerfile.docs Show resolved Hide resolved
Signed-off-by: Chi-Sheng Liu <[email protected]>
@MortalHappiness MortalHappiness force-pushed the feature/containerize-doc-build-env branch from 5cc32c1 to f6e77ad Compare April 19, 2024 09:17
@austin362667
Copy link
Contributor

It works perfectly on my arm-core Mac.
Screenshot 2024-04-19 at 10 32 15 PM

@pingsutw pingsutw merged commit a521dd0 into flyteorg:master Apr 21, 2024
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants