[MINOR][INFRA] Ensure that docs build successfully with SKIP_API=1 #44627
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nchammas
changed the title
nchammas
changed the title
|
cc @HyukjinKwon |
This was referenced Jan 8, 2024
HyukjinKwon
reviewed
Jan 9, 2024
| @@ -761,6 +761,9 @@ jobs: | |||
| run: ./dev/lint-r | |||
| - name: Run documentation build | |||
| run: | | |||
| # Build docs first with SKIP_API to ensure they are buildable without requiring any | |||
| # language docs to be built beforehand. | |||
| cd docs; SKIP_API=1 bundle exec jekyll build; cd .. | |||
There was a problem hiding this comment.
LGTM but just to confirm this doesn't take a lot of time right?
There was a problem hiding this comment.
Correct. This adds around 5 seconds to the "Run documentation build" step.
HyukjinKwon
reviewed
Jan 9, 2024
| @@ -761,6 +761,9 @@ jobs: | |||
| run: ./dev/lint-r | |||
| - name: Run documentation build | |||
| run: | | |||
| # Build docs first with SKIP_API to ensure they are buildable without requiring any | |||
| # language docs to be built beforehand. | |||
| cd docs; SKIP_API=1 bundle exec jekyll build; cd .. | |||
| if [ -f "./dev/is-changed.py" ]; then | |||
| # Skip PySpark and SparkR docs while keeping Scala/Java/SQL docs | |||
| pyspark_modules=`cd dev && python3.9 -c "import sparktestsupport.modules as m; print(','.join(m.name for m in m.all_modules if m.name.startswith('pyspark')))"` | |||
There was a problem hiding this comment.
or wonder if we should set SKIP_SCALADOC=1 together with if [ ./dev/is-changed.py -m ...` condition. That might be better.
There was a problem hiding this comment.
Hmm, could you elaborate a little bit on this? How would this prevent yesterday's scenario?
There was a problem hiding this comment.
Actually let's just merge this. 5 secs won't affect the elapsed time much anyway.
HyukjinKwon
pushed a commit
that referenced
this pull request
Jan 10, 2024
### What changes were proposed in this pull request? Add [custom Jekyll tags][custom] to enable us to conditionally include files in our documentation build in a more user-friendly manner. [This example][example] demonstrates how a custom tag can build on one of Jekyll's built-in tags. [custom]: https://github.com/Shopify/liquid/wiki/Liquid-for-Programmers#create-your-own-tags [example]: Shopify/liquid#370 (comment) Without this change, files have to be included as follows: ```liquid {% for static_file in site.static_files %} {% if static_file.name == 'generated-agg-funcs-table.html' %} {% include_relative generated-agg-funcs-table.html %} {% break %} {% endif %} {% endfor %} ``` With this change, they can be included more intuitively in one of two ways: ```liquid {% include_relative_if_exists generated-agg-funcs-table.html %} {% include_api_gen generated-agg-funcs-table.html %} ``` `include_relative_if_exists` includes a file if it exists and substitutes an HTML comment if not. Use this tag when it's always OK for an include not to exist. `include_api_gen` includes a file if it exists. If it doesn't, it tolerates the missing file only if one of the `SKIP_` flags is set. Otherwise it raises an error. Use this tag for includes that are generated for the language APIs. These files are required to generate complete documentation, but we tolerate their absence during development---i.e. when a skip flag is set. `include_api_gen` will place a visible text placeholder in the document and post a warning to the console to indicate that missing API files are being tolerated. ```sh $ SKIP_API=1 bundle exec jekyll build Configuration file: /Users/nchammas/dev/nchammas/spark/docs/_config.yml Source: /Users/nchammas/dev/nchammas/spark/docs Destination: /Users/nchammas/dev/nchammas/spark/docs/_site Incremental build: disabled. Enable with --incremental Generating... Warning: Tolerating missing API files because the following skip flags are set: SKIP_API done in 1.703 seconds. Auto-regeneration: disabled. Use --watch to enable. ``` This PR supersedes #44393. ### Why are the changes needed? Jekyll does not have a succinct way to [check if a file exists][check], so the required directives to implement such functionality are very cumbersome. We need the ability to do this so that we can [build the docs successfully with `SKIP_API=1`][build], since many includes reference files that are only generated when `SKIP_API` is _not_ set. [check]: jekyll/jekyll#7528 [build]: #44627 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Manually building and reviewing the docs, both with and without `SKIP_API=1`. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #44630 from nchammas/SPARK-46437-conditional-jekyll-include. Authored-by: Nicholas Chammas <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
HyukjinKwon
approved these changes
Jan 10, 2024
|
Merged to master. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This PR tweaks the docs build so that the general docs are first built with
SKIP_API=1to ensure that the docs build works without any language being built beforehand.Why are the changes needed?
Committers expect docs to build with
SKIP_API=1on a fresh checkout. Yet, our CI build does not ensure this. This PR corrects this gap.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Via test commits against this PR.
The build now fails if the docs reference an include that has not been generated yet.
Was this patch authored or co-authored using generative AI tooling?
No.