-
Notifications
You must be signed in to change notification settings - Fork 674
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
Monodocs sphinx build #4347
Monodocs sphinx build #4347
Conversation
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see this done in two stages, which can help with reviewing.
- Move
flyteidl
's docs into this repo. I think this is a majority of this PR. - Move the rest, which I suspect will be a smaller follow up PR.
For step 2, moving flytectl
and flytekit
looks okay to me. I'm concerned over flytesnacks
, because of how many dependencies it adds to building the docs.
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Shoutout to @cosmicBboy. I really like the new doc. Thanks for your tremendous work. |
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4347 +/- ##
==========================================
+ Coverage 58.89% 58.90% +0.01%
==========================================
Files 620 620
Lines 52440 52458 +18
==========================================
+ Hits 30883 30900 +17
- Misses 19090 19091 +1
Partials 2467 2467
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: cosmicBboy <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: cosmicBboy <[email protected]>
Signed-off-by: cosmicBboy <[email protected]>
Signed-off-by: cosmicBboy <[email protected]>
Signed-off-by: cosmicBboy <[email protected]>
Signed-off-by: cosmicBboy <[email protected]>
Signed-off-by: cosmicBboy <[email protected]>
Signed-off-by: cosmicBboy <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, comparing master's build with this PR's build, the build time went from 100 seconds on master
to 448 seconds on this PR. I think that is okay.
@@ -16,4 +16,4 @@ sphinx: | |||
# Optionally set the version of Python and requirements required to build your docs | |||
python: | |||
install: | |||
- requirements: doc-requirements.txt | |||
- requirements: doc-requirements.in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using the pinned version .txt
is safer for doc builds. Otherwise, we can have PRs that break from unrelated issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently this does not work, as pip-compile
will add tensorflow-macos
to the doc-requirements.txt
file when I run it locally, which will not work on readthedocs build.
planning on using conda here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, I do not see tensorflow-macos
in doc-requirements.txt
. Is this change still required?
planning on using conda here
Yes, Conda-lock
will resolve this because it can lock a linux environment from a macos environment.
In any case, I am okay with merging this as is and fixing it later if doc builds become flaky.
- add graphviz as conda dependency - remove monodocs-requirements.{in,txt} Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
Signed-off-by: Niels Bantilan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the merge commits are not signed.
Otherwise, I am okay with the current PR. We can always adjust with follow up PRs.
channels: conda-forge | ||
channel-priority: true | ||
activate-environment: monodocs-env | ||
environment-file: monodocs-environment.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a follow up, I prefer to lock this environment as well. conda-incubator/setup-miniconda
supports lock files: https://github.com/conda-incubator/setup-miniconda#example-7-lockfiles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gonna followup on this with a PR
with: | ||
python-version: "3.8" | ||
- name: Install dependencies | ||
python-version: 3.9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: The python version is already in monodocs-environment.yaml
. Including it here will "override" the version in the environment yaml, so I think it's better to leave it out here:
python-version: 3.9 |
@@ -16,4 +16,4 @@ sphinx: | |||
# Optionally set the version of Python and requirements required to build your docs | |||
python: | |||
install: | |||
- requirements: doc-requirements.txt | |||
- requirements: doc-requirements.in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, I do not see tensorflow-macos
in doc-requirements.txt
. Is this change still required?
planning on using conda here
Yes, Conda-lock
will resolve this because it can lock a linux environment from a macos environment.
In any case, I am okay with merging this as is and fixing it later if doc builds become flaky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! let's merge the PR. 🚀
* rename rsts to docs Signed-off-by: Niels Bantilan <[email protected]> * update docs reqs Signed-off-by: Niels Bantilan <[email protected]> * update reqs, styles Signed-off-by: Niels Bantilan <[email protected]> * update doc requirements Signed-off-by: Niels Bantilan <[email protected]> * edit gitignore Signed-off-by: Niels Bantilan <[email protected]> * update doc requirement Signed-off-by: Niels Bantilan <[email protected]> * update doc requirements Signed-off-by: Niels Bantilan <[email protected]> * update doc requirements Signed-off-by: Niels Bantilan <[email protected]> * minor clean up Signed-off-by: Niels Bantilan <[email protected]> * update deps Signed-off-by: Niels Bantilan <[email protected]> * use doc-requirements.in in .readthedocs.yml Signed-off-by: Niels Bantilan <[email protected]> * updates Signed-off-by: Niels Bantilan <[email protected]> * check in new docs Signed-off-by: Niels Bantilan <[email protected]> * clean up most reference warnings Signed-off-by: Niels Bantilan <[email protected]> * fix ref links Signed-off-by: Niels Bantilan <[email protected]> * update auto examples Signed-off-by: Niels Bantilan <[email protected]> * add logic to find/replace invalid links Signed-off-by: Niels Bantilan <[email protected]> * update docs with changes Signed-off-by: Niels Bantilan <[email protected]> * clean up toc list for deployment docs Signed-off-by: Niels Bantilan <[email protected]> * add back rsts Signed-off-by: Niels Bantilan <[email protected]> * add readthedocs config for monodocs Signed-off-by: Niels Bantilan <[email protected]> * incorporate changes from master Signed-off-by: Niels Bantilan <[email protected]> * reformat concepts section Signed-off-by: Niels Bantilan <[email protected]> * move monodocs readthedocs config Signed-off-by: Niels Bantilan <[email protected]> * debug readthedocs config Signed-off-by: Niels Bantilan <[email protected]> * add monodocs requirements Signed-off-by: Niels Bantilan <[email protected]> * update reqs Signed-off-by: Niels Bantilan <[email protected]> * fix furo dep Signed-off-by: Niels Bantilan <[email protected]> * update logo Signed-off-by: Niels Bantilan <[email protected]> * update logo Signed-off-by: Niels Bantilan <[email protected]> * update css Signed-off-by: Niels Bantilan <[email protected]> * update css Signed-off-by: Niels Bantilan <[email protected]> * update sidebar caption styling Signed-off-by: Niels Bantilan <[email protected]> * update sidebar css * use conda to install monodocs deps Signed-off-by: cosmicBboy <[email protected]> * update conda deps Signed-off-by: cosmicBboy <[email protected]> * update makefile docs target Signed-off-by: cosmicBboy <[email protected]> * add furo dep to environment.yaml Signed-off-by: cosmicBboy <[email protected]> * add retry dep Signed-off-by: cosmicBboy <[email protected]> * update vaex deps Signed-off-by: cosmicBboy <[email protected]> * add graphviz to ci Signed-off-by: cosmicBboy <[email protected]> * add graphviz to readthedocs config Signed-off-by: cosmicBboy <[email protected]> * support pointing to local flytekit, flytesnacks, flytectl - add graphviz as conda dependency - remove monodocs-requirements.{in,txt} Signed-off-by: Niels Bantilan <[email protected]> * delete doc-requirements.txt - defer cleaning this up to followup PR Signed-off-by: Niels Bantilan <[email protected]> * revert doc-requirements.txt Signed-off-by: Niels Bantilan <[email protected]> --------- Signed-off-by: Niels Bantilan <[email protected]> Signed-off-by: cosmicBboy <[email protected]> Co-authored-by: Samhita Alla <[email protected]> Signed-off-by: Paul Dittamo <[email protected]>
Tracking issue
Closes #4329
Describe your changes
This PR adds a custom sphinx build system that imports
flytekit
,flytesnacks
,flytectl
, andflyteidl
documentation and builds the flyte docs as a single docs site. It introduces the following:auto_examples.py
sphinx extension to convert flytesnacks examples into myst markdown. This extension also adds convenience directives to render toctree and table of contents in the flytesnacks examples.import_projects.py
sphinx extension that handles cloning, generating, and rendering docs from imported projects, either from git or from local (e.g.flyteidl
)Check all the applicable boxes