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

auto-generate toctree from flytesnacks index.md docs #4587

Merged
merged 5 commits into from
Dec 13, 2023
Merged

Conversation

cosmicBboy
Copy link
Contributor

@cosmicBboy cosmicBboy commented Dec 12, 2023

Tracking issue

https://github.com/flyteorg/flyte/issues/

Why are the changes needed?

To make the contribution experience for flytesnacks smoother.

What changes were proposed in this pull request?

Updated the import_projects sphinx extension so that it:

  • introduce a custom list-table-toc directive that takes the contents of list-table and auto-generates a toctree.
  • replaces list-table directive with list-table-toc in specific pages: flytesnacks/{userguide, integrations, tutorials}

How was this patch tested?

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Dec 12, 2023
Copy link

codecov bot commented Dec 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2b75785) 58.99% compared to head (eed7d6d) 59.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4587   +/-   ##
=======================================
  Coverage   58.99%   59.00%           
=======================================
  Files         621      621           
  Lines       52498    52498           
=======================================
+ Hits        30969    30974    +5     
+ Misses      19062    19057    -5     
  Partials     2467     2467           
Flag Coverage Δ
unittests 59.00% <ø> (+<0.01%) ⬆️

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.

@neverett
Copy link
Contributor

Should this also include introduction.md, since it contains the Getting Started TOC?

@cosmicBboy
Copy link
Contributor Author

Should this also include introduction.md, since it contains the Getting Started TOC?

There's an equivalent introduction page in flyte monodocs that serves as the main index file: https://flyte-next.readthedocs.io/en/latest/introduction.html

docs/_ext/import_projects.py Outdated Show resolved Hide resolved
@@ -325,12 +324,19 @@
r"{ref}`bioinformatics <bioinformatics>`": r"bioinformatics",
PROTO_REF_PATTERN: PROTO_REF_REPLACE,
r"/protos/docs/service/index": r"/protos/docs/service/service",
r"<weather_forecasting>": r"</flytesnacks/weather_forecasting>",
Copy link
Member

Choose a reason for hiding this comment

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

If flytesnacks adds another ref in docs/tutorials.md, we would still need to add a new entry in flyte. If this is true, can we document this somewhere?

An alternative is to parse tutorials.md and friends for anything that is <SINGLE_WORD> and replace it with <flytesnacks/SINGLE_WORD>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no we don't, the weather_forecasting page is really the only exception to this (tbh it should really be an example). In general all the docs should follow the <auto_examples/... pattern above.

Copy link
Member

Choose a reason for hiding this comment

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

I see weather_forecasting and environment_setup having this pattern. For my understanding, if one adds a <mlops_setup> in docs/tutorials.md do we need to add the following to REPLACE_PATTERNS?

r"<mlops_setup>": r"<flytesnacks/mlops_setup>"

Or we want this to be an anti-pattern and so this should not happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this is an anti-pattern that we need to fix more systematically.

Eventually we can can specify the list-table-toc directive in these files and assume that all refs in these directives follow the pattern <flytesnacks/examples/...>. Any ref outside of list-table-toc won't be treated specially.

Basically any entries listed under list-table-toc in tutorials, userguide, and integrations ought to follow the example template described here: https://docs.flyte.org/projects/cookbook/en/latest/contribute.html

@neverett
Copy link
Contributor

There's an equivalent introduction page in flyte monodocs that serves as the main index file: https://flyte-next.readthedocs.io/en/latest/introduction.html

Gotcha, yeah, I was wondering if we should remove that file from Flyte and build the Getting Started TOC from the file in Flytesnacks, since we'll be making changes to other parts of the Getting Started docs in the next few weeks and months. (We will probably move the Getting Started docs entirely into Flyte, but not for some time.)

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Dec 13, 2023
@cosmicBboy cosmicBboy enabled auto-merge (squash) December 13, 2023 18:21
@cosmicBboy cosmicBboy merged commit 6c84e46 into master Dec 13, 2023
45 checks passed
@cosmicBboy cosmicBboy deleted the auto-gen-toc branch December 13, 2023 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants