This is an automated email from the ASF dual-hosted git repository. potiuk pushed a commit to branch backport-b8be9e5-v3-3-test in repository https://gitbox.apache.org/repos/asf/airflow.git
commit 66b2f43982a9596803b65fb70c18af628230dcac Author: Kaxil Naik <[email protected]> AuthorDate: Tue Sep 22 10:34:07 2026 +0100 Speed up documentation build and spellcheck in CI (#73477) Run Sphinx in-process inside each docs build worker so the fixed per-package startup (autoapi's astroid parsing of the airflow modules, provider.yaml parsing, the airflow import, inventory loading) is paid once per worker instead of once per package. A full 115-package build drops from 645 s to 214 s locally with 4 workers (41.6 to 14.2 CPU-minutes); a small provider now takes about 2 s instead of 17 s. Make the spelling builder write an inventory so a --spellcheck-only run can resolve references to pages added in the same change, and retry only the packages that failed instead of rebuilding everything with html and spelling again. That rebuild-all pass is what turned a 28-minute spellcheck job into a 93-minute one. Also schedule the heaviest packages first with one-at-a-time dispatch, parse each provider.yaml once with a compiled validator and the C YAML loader, and skip the runner disk cleanup for the docs job (93 GB were free before it). Runners are not uniform, so the docs job now passes a 40 GB threshold to the prepare action instead of skipping the cleanup unconditionally. Also document why a spelling build that fails on misspellings still publishes its inventory: under -W Sphinx counts warnings and fails at the end rather than aborting. (cherry picked from commit b8be9e54beef9174c95ca37fa5c4d1e0764ab0cb) --- .../actions/prepare_breeze_and_image/action.yml | 18 +- .github/workflows/ci-image-checks.yml | 3 + devel-common/src/docs/build_docs.py | 99 +++++------ devel-common/src/docs/provider_conf.py | 7 +- .../src/sphinx_exts/airflow_intersphinx.py | 67 +++++-- .../src/sphinx_exts/docs_build/docs_builder.py | 197 +++++++++++++++++---- .../src/sphinx_exts/provider_yaml_utils.py | 42 ++++- .../docs_build/test_build_docs_scheduling.py | 41 +++++ .../sphinx_exts/docs_build/test_docs_builder.py | 146 +++++++++++++++ .../unit/sphinx_exts/test_airflow_intersphinx.py | 113 ++++++++++++ 10 files changed, 625 insertions(+), 108 deletions(-) diff --git a/.github/actions/prepare_breeze_and_image/action.yml b/.github/actions/prepare_breeze_and_image/action.yml index 6e6c4efd205..1815b2a743c 100644 --- a/.github/actions/prepare_breeze_and_image/action.yml +++ b/.github/actions/prepare_breeze_and_image/action.yml @@ -34,6 +34,14 @@ inputs: make-mnt-writeable-and-cleanup: description: 'Whether to cleanup /mnt' required: true + min-free-space-gb: + description: > + Skip removing the runner's unused pre-installed tooling (close to three minutes) when the root + filesystem already has at least this many GB free. Empty means always remove it. Runners are + not uniform, so a job whose footprint is only the CI image (docs build) passes a threshold + rather than skipping unconditionally. + required: false + default: "" outputs: host-python-version: description: Python version used in host @@ -47,7 +55,15 @@ runs: if: inputs.make-mnt-writeable-and-cleanup == 'true' - name: "Free up disk space" shell: bash - run: ./scripts/tools/free_up_disk_space.sh + env: + MIN_FREE_SPACE_GB: ${{ inputs.min-free-space-gb }} + run: | + FREE_GB=$(df --output=avail --block-size=1G / | tail -1 | tr -d ' ') + if [[ -n "${MIN_FREE_SPACE_GB}" && "${FREE_GB}" -ge "${MIN_FREE_SPACE_GB}" ]]; then + echo "${FREE_GB} GB free on / (>= ${MIN_FREE_SPACE_GB} GB), skipping the disk cleanup" + else + ./scripts/tools/free_up_disk_space.sh + fi - name: "Install Breeze" uses: ./.github/actions/breeze id: breeze diff --git a/.github/workflows/ci-image-checks.yml b/.github/workflows/ci-image-checks.yml index aa5b75d52ed..87afc3c8a1f 100644 --- a/.github/workflows/ci-image-checks.yml +++ b/.github/workflows/ci-image-checks.yml @@ -193,6 +193,9 @@ jobs: python: "${{ inputs.default-python-version }}" use-uv: ${{ inputs.use-uv }} make-mnt-writeable-and-cleanup: true + # The docs build needs ~25 GB (image tar, loaded image, built docs); runners usually + # start with ~90 GB free, in which case the 3-minute cleanup is skipped. + min-free-space-gb: 40 - name: "Restore docs inventory cache" uses: apache/infrastructure-actions/stash/restore@ec1b354a0455b41001d77473236d02a0ee0adba4 with: diff --git a/devel-common/src/docs/build_docs.py b/devel-common/src/docs/build_docs.py index 860df0115c7..54ddeb3102b 100755 --- a/devel-common/src/docs/build_docs.py +++ b/devel-common/src/docs/build_docs.py @@ -38,7 +38,7 @@ from rich.console import Console from tabulate import tabulate from sphinx_exts.docs_build import dev_index_generator -from sphinx_exts.docs_build.code_utils import CONSOLE_WIDTH, GENERATED_PATH +from sphinx_exts.docs_build.code_utils import AIRFLOW_CONTENT_ROOT_PATH, CONSOLE_WIDTH, GENERATED_PATH from sphinx_exts.docs_build.docs_builder import ( AirflowDocsBuilder, get_available_packages, @@ -328,6 +328,34 @@ def print_build_output(result: BuildDocsResult): console.print(f"[bright_blue]{result.package_name:60}: " + "#" * 80) +_API_SOURCE_ROOTS = { + "apache-airflow": AIRFLOW_CONTENT_ROOT_PATH / "airflow-core" / "src", + "task-sdk": AIRFLOW_CONTENT_ROOT_PATH / "task-sdk" / "src", + "apache-airflow-ctl": AIRFLOW_CONTENT_ROOT_PATH / "airflow-ctl" / "src", +} + + +def _estimated_build_weight(package_name: str) -> int: + """Number of Python files autoapi will parse: the rough cost of building one package.""" + builder = AirflowDocsBuilder(package_name=package_name) + source_root = ( + builder.provider_path / "src" if builder.is_provider else _API_SOURCE_ROOTS.get(package_name) + ) + if source_root is None or not source_root.exists(): + return 0 + return sum(1 for _ in source_root.rglob("*.py")) + + +def sort_heaviest_first(packages: list[str]) -> list[str]: + """ + Order packages so the long builds start first. + + With a handful of workers, starting google/amazon/apache-airflow last would leave the other workers + idle at the end; starting them first lets the many small packages fill in around them. + """ + return sorted(packages, key=lambda package_name: (-_estimated_build_weight(package_name), package_name)) + + def run_docs_build_in_parallel( all_build_errors: dict[str, list[DocBuildError]], packages_to_build: list[str], @@ -337,7 +365,7 @@ def run_docs_build_in_parallel( """Runs documentation building in parallel.""" doc_build_specifications: list[BuildSpecification] = [] with with_group("Scheduling documentation to build"): - for package_name in packages_to_build: + for package_name in sort_heaviest_first(packages_to_build): console.print(f"[bright_blue]{package_name:60}:[/] Scheduling documentation to build") doc_build_specifications.append( BuildSpecification( @@ -348,7 +376,11 @@ def run_docs_build_in_parallel( ) with with_group("Running docs building"): console.print() - result_list = pool.map(perform_docs_build_for_single_package, doc_build_specifications) + # chunksize=1 hands packages out one at a time, so a worker that finishes early picks up the + # next package instead of the fixed chunk pool.map would have pre-assigned to it. + result_list = list( + pool.imap_unordered(perform_docs_build_for_single_package, doc_build_specifications, chunksize=1) + ) for result in result_list: if result.errors: all_build_errors[result.package_name].extend(result.errors) @@ -377,14 +409,18 @@ def run_spell_check_in_parallel( """Runs spell check in parallel.""" spell_check_specifications: list[BuildSpecification] = [] with with_group("Scheduling spell checking of documentation"): - for package_name in packages_to_build: + for package_name in sort_heaviest_first(packages_to_build): console.print(f"[bright_blue]{package_name:60}:[/] Scheduling spellchecking") spell_check_specifications.append( BuildSpecification(package_name=package_name, is_autobuild=False, verbose=verbose) ) with with_group("Running spell checking of documentation"): console.print() - result_list = pool.map(perform_spell_check_for_single_package, spell_check_specifications) + result_list = list( + pool.imap_unordered( + perform_spell_check_for_single_package, spell_check_specifications, chunksize=1 + ) + ) for result in result_list: if result.spelling_errors: all_spelling_errors[result.package_name].extend(result.spelling_errors) @@ -693,52 +729,19 @@ def build_docs( all_spelling_errors.update(package_spelling_errors) if not one_pass_only: - # Build documentation for some packages again if it can help them. - package_build_errors = retry_building_docs_if_needed( - all_build_errors=all_build_errors, - all_spelling_errors=all_spelling_errors, - autobuild=autobuild, - docs_only=docs_only, - jobs=jobs, - verbose=verbose, - package_build_errors=package_build_errors, - originally_built_packages=packages_to_build, - # If spellchecking fails, we need to rebuild all packages first in case some references - # are broken between packages - rebuild_all_packages=spellcheck_only, - ) - - # And try again in case one change spans across three-level dependencies. - package_build_errors = retry_building_docs_if_needed( - all_build_errors=all_build_errors, - all_spelling_errors=all_spelling_errors, - autobuild=autobuild, - docs_only=docs_only, - jobs=jobs, - verbose=verbose, - package_build_errors=package_build_errors, - originally_built_packages=packages_to_build, - # In the 3rd pass we only rebuild packages that failed in the 2nd pass - # no matter if we do spellcheck-only build - rebuild_all_packages=False, - ) - - if spellcheck_only: - # And in case of spellcheck-only, we add a 4th pass to account for A->B-C case - # For spellcheck-only build, the first pass does not solve any of the dependency - # Issues, they only start getting solved and the 2nd pass so we might need to do one more pass + # Packages that failed on a cross-reference to a package built later in the same pass are + # built again now that the inventory exists (spelling builds write one too, see + # airflow_intersphinx). The second retry covers a change spanning A -> B -> C dependencies. + for _ in range(2): package_build_errors = retry_building_docs_if_needed( all_build_errors=all_build_errors, all_spelling_errors=all_spelling_errors, autobuild=autobuild, docs_only=docs_only, + spellcheck_only=spellcheck_only, jobs=jobs, verbose=verbose, package_build_errors=package_build_errors, - originally_built_packages=packages_to_build, - # In the 4th pass we only rebuild packages that failed in the 3rd pass - # no matter if we do spellcheck-only build - rebuild_all_packages=False, ) dev_index_generator.generate_index(f"{GENERATED_PATH}/_build/index.html") @@ -758,11 +761,10 @@ def retry_building_docs_if_needed( all_spelling_errors: dict[str, list[SpellingError]], autobuild: bool, docs_only: bool, + spellcheck_only: bool, jobs: int, verbose: bool, package_build_errors: dict[str, list[DocBuildError]], - originally_built_packages: list[str], - rebuild_all_packages: bool, ) -> dict[str, list[DocBuildError]]: to_retry_packages = [ package_name @@ -773,11 +775,6 @@ def retry_building_docs_if_needed( console.print("[green]No packages to retry. No more passes are needed.[/]") return package_build_errors console.print("[warning] Some packages failed to build due to dependencies. We need another pass.[/]") - # if we are rebuilding all packages, we need to retry all packages - # even if there is one package to rebuild only - if rebuild_all_packages: - console.print("[warning]Rebuilding all originally built package as this is the first build pass:[/]") - to_retry_packages = originally_built_packages console.print(f"[bright_blue]Packages to rebuild: {to_retry_packages}[/]") for package_name in to_retry_packages: if package_name in all_build_errors: @@ -788,7 +785,7 @@ def retry_building_docs_if_needed( packages_to_build=to_retry_packages, is_autobuild=autobuild, docs_only=docs_only, - spellcheck_only=False, + spellcheck_only=spellcheck_only, jobs=jobs, verbose=verbose, ) diff --git a/devel-common/src/docs/provider_conf.py b/devel-common/src/docs/provider_conf.py index 96d5e2f93cc..4df107b7d41 100644 --- a/devel-common/src/docs/provider_conf.py +++ b/devel-common/src/docs/provider_conf.py @@ -293,7 +293,12 @@ autoapi_dirs = [BASE_PROVIDER_SRC_PATH.as_posix()] autoapi_ignore = BASIC_AUTOAPI_IGNORE_PATTERNS autoapi_log = logging.getLogger("sphinx.autoapi.mappers.base") -autoapi_log.addFilter(filter_autoapi_ignore_entries) +# One process builds several packages in a row and re-imports this module each time, so guard +# against stacking the same filter on the (process-wide) logger. +if not any( + getattr(f, "__name__", None) == filter_autoapi_ignore_entries.__name__ for f in autoapi_log.filters +): + autoapi_log.addFilter(filter_autoapi_ignore_entries) autoapi_python_use_implicit_namespaces = True diff --git a/devel-common/src/sphinx_exts/airflow_intersphinx.py b/devel-common/src/sphinx_exts/airflow_intersphinx.py index 41f8d36659b..a5b83bfcff4 100644 --- a/devel-common/src/sphinx_exts/airflow_intersphinx.py +++ b/devel-common/src/sphinx_exts/airflow_intersphinx.py @@ -22,12 +22,61 @@ from pathlib import Path from typing import TYPE_CHECKING, Any from provider_yaml_utils import load_package_data +from sphinx.util.inventory import InventoryFile if TYPE_CHECKING: from sphinx.application import Sphinx AIRFLOW_ROOT_PATH = Path(os.path.abspath(__file__)).parents[3] GENERATED_PATH = AIRFLOW_ROOT_PATH / "generated" +INVENTORY_FILENAME = "objects.inv" + + +def _inventory_for(package_name: str, versioned: bool) -> Path: + """ + Prefer an inventory written earlier in this run, fall back to the one fetched from the published docs. + + The html builder writes its inventory at the root of the build directory; a spelling-only build + writes one into its own output directory (see ``_dump_inventory_after_spelling_build``, the + directory name mirrors ``AirflowDocsBuilder.log_spelling_output_dir``). + """ + build_dir = GENERATED_PATH / "_build" / "docs" / package_name / ("stable" if versioned else "") + built_this_run = ( + build_dir / INVENTORY_FILENAME, + build_dir / f"output-spelling-results-{package_name}" / INVENTORY_FILENAME, + ) + downloaded = GENERATED_PATH / "_inventory_cache" / package_name / INVENTORY_FILENAME + return next((inventory for inventory in built_this_run if inventory.exists()), downloaded) + + +class _HtmlTargetUris: + """Gives inventory entries the URIs the html build would, so the file is an ordinary inventory.""" + + @staticmethod + def get_target_uri(docname: str, typ: str | None = None) -> str: + return f"{docname}.html" + + +def _dump_inventory_after_spelling_build(app: Sphinx, exception: Exception | None) -> None: + """ + Write ``objects.inv`` after a successful spelling build. + + Cross-references are resolved by every builder, so a ``--spellcheck-only`` run of package B fails + on a label that package A added in the same change unless A's fresh inventory is available. Only + the html builder writes inventories, which used to force the spellcheck run to rebuild every + package a second time with html. Writing the inventory here lets a spellcheck-only run resolve + references the same way a docs build does. + + "Successful" means the build ran to completion, not that it passed: under ``-W`` Sphinx (8.1 and + 9.x) counts warnings and sets a non-zero exit status at the end instead of aborting, so a package + with a genuine misspelling or an unresolved reference still reaches this hook with + ``exception=None`` and still publishes its complete inventory for the packages that depend on it. + Only a build that crashed part-way (``exception`` set) is skipped, because its environment may + not hold every document. + """ + if exception is not None or app.builder.name != "spelling": + return + InventoryFile.dump(os.path.join(app.outdir, INVENTORY_FILENAME), app.env, _HtmlTargetUris()) # type: ignore[arg-type] def _create_init_py(app, config): @@ -51,39 +100,32 @@ def _generate_provider_intersphinx_mapping() -> dict[str, tuple[str, tuple[str, continue provider_base_url = f"/docs/{package_name}/{current_version}/" - doc_inventory = GENERATED_PATH / "_build" / "docs" / package_name / current_version / "objects.inv" - cache_inventory = GENERATED_PATH / "_inventory_cache" / package_name / "objects.inv" + inventory = _inventory_for(package_name, versioned=True) # Skip adding the mapping if the path does not exist - if not os.path.exists(doc_inventory) and not os.path.exists(cache_inventory): + if not inventory.exists(): continue airflow_mapping[package_name] = ( # base URI provider_base_url, - (doc_inventory.as_posix() if doc_inventory.exists() else cache_inventory.as_posix(),), + (inventory.as_posix(),), ) for pkg_name in ["apache-airflow", "helm-chart", "task-sdk"]: if os.environ.get("AIRFLOW_PACKAGE_NAME") == pkg_name: continue - doc_inventory = GENERATED_PATH / "_build" / "docs" / pkg_name / current_version / "objects.inv" - cache_inventory = GENERATED_PATH / "_inventory_cache" / pkg_name / "objects.inv" - airflow_mapping[pkg_name] = ( # base URI f"/docs/{pkg_name}/stable/", - (doc_inventory.as_posix() if doc_inventory.exists() else cache_inventory.as_posix(),), + (_inventory_for(pkg_name, versioned=True).as_posix(),), ) for pkg_name in ["apache-airflow-providers", "docker-stack"]: if os.environ.get("AIRFLOW_PACKAGE_NAME") == pkg_name: continue - doc_inventory = GENERATED_PATH / "_build" / "docs" / pkg_name / "objects.inv" - cache_inventory = GENERATED_PATH / "_inventory_cache" / pkg_name / "objects.inv" - airflow_mapping[pkg_name] = ( # base URI f"/docs/{pkg_name}/", - (doc_inventory.as_posix() if doc_inventory.exists() else cache_inventory.as_posix(),), + (_inventory_for(pkg_name, versioned=False).as_posix(),), ) return airflow_mapping @@ -91,6 +133,7 @@ def _generate_provider_intersphinx_mapping() -> dict[str, tuple[str, tuple[str, def setup(app: Sphinx): """Sets the plugin up""" app.connect("config-inited", _create_init_py) + app.connect("build-finished", _dump_inventory_after_spelling_build) return {"version": "builtin", "parallel_read_safe": True, "parallel_write_safe": True} diff --git a/devel-common/src/sphinx_exts/docs_build/docs_builder.py b/devel-common/src/sphinx_exts/docs_build/docs_builder.py index 147215a7353..a7df0ad6a10 100644 --- a/devel-common/src/sphinx_exts/docs_build/docs_builder.py +++ b/devel-common/src/sphinx_exts/docs_build/docs_builder.py @@ -16,21 +16,27 @@ # under the License. from __future__ import annotations +import contextlib import os import re import shlex import shutil +import signal import sys +import threading +from collections.abc import Iterator from pathlib import Path from subprocess import run from rich.console import Console +from sphinx.cmd.build import build_main from sphinx_exts.docs_build.code_utils import ( AIRFLOW_CONTENT_ROOT_PATH, ALL_PROVIDER_YAMLS, ALL_PROVIDER_YAMLS_WITH_SUSPENDED, CONSOLE_WIDTH, + DOCS_SOURCES_PATH, GENERATED_PATH, PROCESS_TIMEOUT, ) @@ -39,6 +45,121 @@ from sphinx_exts.docs_build.spelling_checks import SpellingError, parse_spelling console = Console(force_terminal=True, color_system="standard", width=CONSOLE_WIDTH) +# Sphinx is run in the current process (one worker builds many packages in a row) so that the fixed +# start-up cost of a build is paid once per worker instead of once per package. For a small provider +# that fixed cost is ~20s out of ~22s: importing airflow, parsing and validating every provider.yaml, +# and - the largest part - autoapi's astroid parsing of the airflow modules every provider imports. +# The astroid parse cache in particular stays warm across the packages a worker builds. + + +def _forget_sphinx_conf_modules() -> None: + """ + Drop the shared Sphinx configuration modules so the next conf.py re-executes them from scratch. + + The per-package ``conf.py`` files do ``from docs.provider_conf import *`` (or import + ``docs.utils.conf_constants``) and then mutate the lists they get - ``extensions.append(...)``, + ``autoapi_ignore.extend(...)``. Reusing the cached module would leak one package's additions into + the next build, so everything under ``devel-common/src/docs`` except the build script is forgotten. + """ + docs_sources_prefix = DOCS_SOURCES_PATH.as_posix() + for name, module in list(sys.modules.items()): + if name == "docs.build_docs" or not name.startswith("docs."): + continue + module_file = getattr(module, "__file__", None) or "" + if module_file.startswith(docs_sources_prefix): + del sys.modules[name] + + +class _RedirectableStream: + """ + Stand-in for ``sys.stdout`` / ``sys.stderr`` that lives as long as the worker process. + + Libraries keep a reference to whatever stream is current when they first need one (docutils' + ``Reporter`` in sphinx-argparse's nested parser is one example). Redirecting straight to a + per-package log file would leave such references pointing at a closed file once that package is + done and crash the next build with "I/O operation on closed file". This object never closes; it + only changes where it writes: the current package's log file during a build, the worker's real + stream otherwise. + """ + + def __init__(self, fallback) -> None: + self._fallback = fallback + self.target = None + + def _stream(self): + return self.target if self.target is not None else self._fallback + + def write(self, data: str) -> int: + return self._stream().write(data) + + def flush(self) -> None: + self._stream().flush() + + def isatty(self) -> bool: + return False + + @property + def encoding(self) -> str: + return getattr(self._stream(), "encoding", None) or "utf-8" + + +_STDOUT_PROXY = _RedirectableStream(sys.stdout) +_STDERR_PROXY = _RedirectableStream(sys.stderr) + + [email protected] +def _output_to(log_file) -> Iterator[None]: + """Send everything written to stdout/stderr during the block to ``log_file``.""" + _STDOUT_PROXY.target = log_file + _STDERR_PROXY.target = log_file + try: + with contextlib.redirect_stdout(_STDOUT_PROXY), contextlib.redirect_stderr(_STDERR_PROXY): + yield + finally: + _STDOUT_PROXY.target = None + _STDERR_PROXY.target = None + + [email protected] +def _working_directory(path: Path) -> Iterator[None]: + previous = os.getcwd() + os.chdir(path) + try: + yield + finally: + os.chdir(previous) + + [email protected] +def _sys_path_prepended(paths: list[Path]) -> Iterator[None]: + entries = [path.as_posix() for path in paths] + sys.path[:0] = entries + try: + yield + finally: + for entry in entries: + with contextlib.suppress(ValueError): + sys.path.remove(entry) + + [email protected] +def _build_timeout(seconds: int) -> Iterator[None]: + """Abort a runaway in-process build the way the previous subprocess timeout did.""" + if not hasattr(signal, "SIGALRM") or threading.current_thread() is not threading.main_thread(): + yield + return + + def _raise_timeout(signum, frame): + raise TimeoutError(f"Sphinx build did not finish within {seconds} seconds") + + previous_handler = signal.signal(signal.SIGALRM, _raise_timeout) + signal.alarm(seconds) + try: + yield + finally: + signal.alarm(0) + signal.signal(signal.SIGALRM, previous_handler) + class AirflowDocsBuilder: """Documentation builder for Airflow.""" @@ -181,25 +302,12 @@ class AirflowDocsBuilder: ] if os.environ.get("CI", "") != "true" and verbose: console.print("[yellow]Command to run:[/] ", " ".join([shlex.quote(arg) for arg in build_cmd])) - env = os.environ.copy() - env["AIRFLOW_PACKAGE_NAME"] = self.package_name - if self.pythonpath: - env["PYTHONPATH"] = ":".join([path.as_posix() for path in self.pythonpath]) if verbose: console.print( f"[bright_blue]{self.package_name:60}:[/] The output is hidden until an error occurs." ) - with open(self.log_spelling_filename, "w") as output: - completed_proc = run( - build_cmd, - check=False, - cwd=AIRFLOW_CONTENT_ROOT_PATH, - env=env, - stdout=output if not verbose else None, - stderr=output if not verbose else None, - timeout=PROCESS_TIMEOUT, - ) - if completed_proc.returncode != 0: + returncode = self._run_sphinx(build_cmd, log_file=self.log_spelling_filename, verbose=verbose) + if returncode != 0: spelling_errors.append( SpellingError( file_path=None, @@ -207,9 +315,7 @@ class AirflowDocsBuilder: spelling=None, suggestion=None, context_line=None, - message=( - f"Sphinx spellcheck returned non-zero exit status: {completed_proc.returncode}." - ), + message=f"Sphinx spellcheck returned non-zero exit status: {returncode}.", ) ) spelling_warning_text = "" @@ -263,31 +369,18 @@ class AirflowDocsBuilder: ] if os.environ.get("CI", "") != "true" and verbose: console.print("[yellow]Command to run:[/] ", " ".join([shlex.quote(arg) for arg in build_cmd])) - env = os.environ.copy() - env["AIRFLOW_PACKAGE_NAME"] = self.package_name - if self.pythonpath: - env["PYTHONPATH"] = ":".join([path.as_posix() for path in self.pythonpath]) if verbose: console.print( f"[bright_blue]{self.package_name:60}:[/] Running sphinx. " f"The output is hidden until an error occurs." ) - with open(self.log_build_filename, "w") as output: - completed_proc = run( - build_cmd, - check=False, - cwd=AIRFLOW_CONTENT_ROOT_PATH, - env=env, - stdout=output if not verbose else None, - stderr=output if not verbose else None, - timeout=PROCESS_TIMEOUT, - ) - if completed_proc.returncode != 0: + returncode = self._run_sphinx(build_cmd, log_file=self.log_build_filename, verbose=verbose) + if returncode != 0: build_errors.append( DocBuildError( file_path=None, line_no=None, - message=f"Sphinx returned non-zero exit status: {completed_proc.returncode}.", + message=f"Sphinx returned non-zero exit status: {returncode}.", ) ) if self.log_build_warning_filename.is_file(): @@ -308,6 +401,42 @@ class AirflowDocsBuilder: def get_command(self) -> str: return "sphinx-autobuild" if self.is_autobuild else "sphinx-build" + def _run_sphinx(self, build_cmd: list[str], *, log_file: Path, verbose: bool) -> int: + """ + Run a ``sphinx-build`` / ``sphinx-autobuild`` command line and return its exit status. + + ``sphinx-build`` runs in the current process (see the module comment for why); its output goes + to ``log_file`` unless ``verbose`` is set. ``sphinx-autobuild`` is a long-running server and + keeps running as a subprocess. + """ + if self.is_autobuild: + env = os.environ.copy() + env["AIRFLOW_PACKAGE_NAME"] = self.package_name + if self.pythonpath: + env["PYTHONPATH"] = ":".join([path.as_posix() for path in self.pythonpath]) + with open(log_file, "w") as output: + completed_proc = run( + build_cmd, + check=False, + cwd=AIRFLOW_CONTENT_ROOT_PATH, + env=env, + stdout=output if not verbose else None, + stderr=output if not verbose else None, + timeout=PROCESS_TIMEOUT, + ) + return completed_proc.returncode + _forget_sphinx_conf_modules() + os.environ["AIRFLOW_PACKAGE_NAME"] = self.package_name + with ( + open(log_file, "w") as output, + _output_to(output) if not verbose else contextlib.nullcontext(), + _working_directory(AIRFLOW_CONTENT_ROOT_PATH), + _sys_path_prepended(self.pythonpath), + _build_timeout(PROCESS_TIMEOUT), + ): + # Sphinx reports a TimeoutError raised by the alarm like any other build failure. + return build_main(build_cmd[1:]) + def get_available_providers_distributions(include_suspended: bool = False): """Get list of all available providers packages to build.""" diff --git a/devel-common/src/sphinx_exts/provider_yaml_utils.py b/devel-common/src/sphinx_exts/provider_yaml_utils.py index 9fc02b61f3c..324876be6fd 100644 --- a/devel-common/src/sphinx_exts/provider_yaml_utils.py +++ b/devel-common/src/sphinx_exts/provider_yaml_utils.py @@ -23,6 +23,7 @@ from pathlib import Path from typing import Any import jsonschema +import jsonschema.validators import yaml AIRFLOW_ROOT_PATH = Path(__file__).parents[3].resolve() @@ -31,6 +32,8 @@ AIRFLOW_PROVIDERS_SRC = AIRFLOW_PROVIDERS_PATH / "src" PROVIDER_DATA_SCHEMA_PATH = ( AIRFLOW_ROOT_PATH / "airflow-core" / "src" / "airflow" / "provider.yaml.schema.json" ) +# The C loader parses YAML several times faster than the pure-Python one; fall back when libyaml is absent. +_YAML_LOADER = getattr(yaml, "CSafeLoader", yaml.SafeLoader) @cache @@ -65,24 +68,31 @@ def get_all_provider_yaml_paths() -> list[Path]: @cache -def load_package_data(include_suspended: bool = False) -> list[dict[str, Any]]: - """ - Load all data from providers files +def _provider_yaml_validator() -> jsonschema.protocols.Validator: + """Return a validator compiled once for the provider.yaml schema. - :return: A list containing the contents of all provider.yaml files - old and new structure. + ``jsonschema.validate`` re-validates the schema itself on every call, which is the dominant + cost when validating ~100 provider.yaml files in a row (several seconds per Sphinx process). """ schema = provider_yaml_schema() + validator_cls = jsonschema.validators.validator_for(schema) + validator_cls.check_schema(schema) + return validator_cls(schema) + + +@cache +def _load_all_provider_yamls() -> tuple[dict[str, Any], ...]: + """Parse and validate every provider.yaml once per process, suspended providers included.""" + validator = _provider_yaml_validator() result = [] for provider_yaml_path in get_all_provider_yaml_paths(): with open(provider_yaml_path) as yaml_file: - provider = yaml.safe_load(yaml_file) + provider = yaml.load(yaml_file, Loader=_YAML_LOADER) try: - jsonschema.validate(provider, schema=schema) + validator.validate(provider) except jsonschema.ValidationError as ex: msg = f"Unable to parse: {provider_yaml_path}. Original error {type(ex).__name__}: {ex}" raise RuntimeError(msg) - if provider["state"] == "suspended" and not include_suspended: - continue provider_yaml_dir_str = os.path.dirname(provider_yaml_path) module = provider["package-name"][len("apache-") :].replace("-", ".") module_folder = module[len("airflow-providers-") :].replace(".", "/") @@ -91,4 +101,18 @@ def load_package_data(include_suspended: bool = False) -> list[dict[str, Any]]: provider["docs-dir"] = os.path.dirname(provider_yaml_path.parent / "docs") provider["system-tests-dir"] = f"{provider_yaml_dir_str}/tests/system/{module_folder}" result.append(provider) - return result + return tuple(result) + + +@cache +def load_package_data(include_suspended: bool = False) -> list[dict[str, Any]]: + """ + Load all data from providers files + + :return: A list containing the contents of all provider.yaml files - old and new structure. + """ + return [ + provider + for provider in _load_all_provider_yamls() + if include_suspended or provider["state"] != "suspended" + ] diff --git a/devel-common/tests/unit/sphinx_exts/docs_build/test_build_docs_scheduling.py b/devel-common/tests/unit/sphinx_exts/docs_build/test_build_docs_scheduling.py new file mode 100644 index 00000000000..5e827401061 --- /dev/null +++ b/devel-common/tests/unit/sphinx_exts/docs_build/test_build_docs_scheduling.py @@ -0,0 +1,41 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from unittest import mock + +from docs import build_docs + + [email protected](build_docs, "_estimated_build_weight", autospec=True) +def test_sort_heaviest_first_orders_by_weight_then_name(mock_weight): + weights = {"google": 3000, "amazon": 1500, "ftp": 20, "ssh": 20, "docker-stack": 5} + mock_weight.side_effect = weights.__getitem__ + + ordered = build_docs.sort_heaviest_first(["ssh", "docker-stack", "google", "ftp", "amazon"]) + + assert ordered == ["google", "amazon", "ftp", "ssh", "docker-stack"] + + +def test_estimated_build_weight_ranks_real_packages_sensibly(): + google = build_docs._estimated_build_weight("apache-airflow-providers-google") + core = build_docs._estimated_build_weight("apache-airflow") + ftp = build_docs._estimated_build_weight("apache-airflow-providers-ftp") + + assert google > ftp > 0 + assert core > ftp + assert build_docs._estimated_build_weight("docker-stack") == 0 diff --git a/devel-common/tests/unit/sphinx_exts/docs_build/test_docs_builder.py b/devel-common/tests/unit/sphinx_exts/docs_build/test_docs_builder.py new file mode 100644 index 00000000000..bd68b2af52d --- /dev/null +++ b/devel-common/tests/unit/sphinx_exts/docs_build/test_docs_builder.py @@ -0,0 +1,146 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import io +import os +import subprocess +import sys +import types +from unittest import mock + +from sphinx_exts.docs_build import docs_builder +from sphinx_exts.docs_build.code_utils import AIRFLOW_CONTENT_ROOT_PATH, DOCS_SOURCES_PATH +from sphinx_exts.docs_build.docs_builder import ( + AirflowDocsBuilder, + _forget_sphinx_conf_modules, + _RedirectableStream, +) + + +class TestRedirectableStream: + def test_writes_go_to_target_while_set_and_to_fallback_otherwise(self): + fallback = io.StringIO() + target = io.StringIO() + stream = _RedirectableStream(fallback) + + stream.write("before\n") + stream.target = target + stream.write("during\n") + stream.target = None + stream.write("after\n") + + assert fallback.getvalue() == "before\nafter\n" + assert target.getvalue() == "during\n" + + def test_reference_kept_from_an_earlier_build_stays_usable_after_its_log_is_closed(self): + """A library that cached the stream during build 1 must not hit a closed file in build 2.""" + fallback = io.StringIO() + stream = _RedirectableStream(fallback) + first_log = io.StringIO() + stream.target = first_log + cached_by_library = stream + stream.target = None + first_log.close() + second_log = io.StringIO() + stream.target = second_log + + cached_by_library.write("from build 2") + + assert second_log.getvalue() == "from build 2" + assert stream.isatty() is False + assert stream.encoding + + +class TestForgetSphinxConfModules: + def test_forgets_conf_modules_but_keeps_build_script_and_unrelated_modules(self, monkeypatch): + conf_module = types.ModuleType("docs.provider_conf") + conf_module.__file__ = (DOCS_SOURCES_PATH / "provider_conf.py").as_posix() + constants_module = types.ModuleType("docs.utils.conf_constants") + constants_module.__file__ = (DOCS_SOURCES_PATH / "utils" / "conf_constants.py").as_posix() + build_script = types.ModuleType("docs.build_docs") + build_script.__file__ = (DOCS_SOURCES_PATH / "build_docs.py").as_posix() + elsewhere = types.ModuleType("docs.elsewhere") + elsewhere.__file__ = "/somewhere/else/docs/elsewhere.py" + for module in (conf_module, constants_module, build_script, elsewhere): + monkeypatch.setitem(sys.modules, module.__name__, module) + + _forget_sphinx_conf_modules() + + assert "docs.provider_conf" not in sys.modules + assert "docs.utils.conf_constants" not in sys.modules + assert sys.modules["docs.build_docs"] is build_script + assert sys.modules["docs.elsewhere"] is elsewhere + + +class TestRunSphinxInProcess: + def test_runs_build_main_with_output_in_log_file_and_restores_process_state(self, tmp_path, monkeypatch): + builder = AirflowDocsBuilder(package_name="apache-airflow-providers-ftp") + log_file = tmp_path / "build.log" + previous_cwd = os.getcwd() + previous_sys_path = list(sys.path) + seen: dict[str, object] = {} + + def fake_build_main(argv): + seen["argv"] = list(argv) + seen["cwd"] = os.getcwd() + seen["package"] = os.environ.get("AIRFLOW_PACKAGE_NAME") + print("sphinx says hi") + print("sphinx warns", file=sys.stderr) + return 0 + + monkeypatch.setattr( + docs_builder, + "build_main", + mock.create_autospec(docs_builder.build_main, side_effect=fake_build_main), + ) + monkeypatch.setattr( + docs_builder, + "_forget_sphinx_conf_modules", + mock.create_autospec(docs_builder._forget_sphinx_conf_modules), + ) + + returncode = builder._run_sphinx( + ["sphinx-build", "-T", "-b", "html", "src", "out"], log_file=log_file, verbose=False + ) + + assert returncode == 0 + assert seen["argv"] == ["-T", "-b", "html", "src", "out"] + assert seen["cwd"] == AIRFLOW_CONTENT_ROOT_PATH.as_posix() + assert seen["package"] == "apache-airflow-providers-ftp" + assert log_file.read_text() == "sphinx says hi\nsphinx warns\n" + assert os.getcwd() == previous_cwd + assert sys.path == previous_sys_path + docs_builder._forget_sphinx_conf_modules.assert_called_once_with() + + def test_autobuild_stays_a_subprocess(self, tmp_path, monkeypatch): + builder = AirflowDocsBuilder(package_name="apache-airflow-providers-ftp") + builder.is_autobuild = True + run = mock.create_autospec( + docs_builder.run, return_value=mock.Mock(spec=subprocess.CompletedProcess, returncode=3) + ) + monkeypatch.setattr(docs_builder, "run", run) + monkeypatch.setattr(docs_builder, "build_main", mock.create_autospec(docs_builder.build_main)) + + returncode = builder._run_sphinx( + ["sphinx-autobuild", "src", "out"], log_file=tmp_path / "log", verbose=False + ) + + assert returncode == 3 + run.assert_called_once() + assert run.call_args.args[0] == ["sphinx-autobuild", "src", "out"] + docs_builder.build_main.assert_not_called() diff --git a/devel-common/tests/unit/sphinx_exts/test_airflow_intersphinx.py b/devel-common/tests/unit/sphinx_exts/test_airflow_intersphinx.py new file mode 100644 index 00000000000..ce5b77ad9ba --- /dev/null +++ b/devel-common/tests/unit/sphinx_exts/test_airflow_intersphinx.py @@ -0,0 +1,113 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import sys +from pathlib import Path +from unittest import mock + +import pytest +from sphinx.application import Sphinx +from sphinx.builders import Builder +from sphinx.environment import BuildEnvironment +from sphinx.errors import SphinxError + +SPHINX_EXTS_PATH = Path(__file__).parents[3] / "src" / "sphinx_exts" +if SPHINX_EXTS_PATH.as_posix() not in sys.path: + # The extensions are loaded by Sphinx from this directory and import each other by bare name. + sys.path.append(SPHINX_EXTS_PATH.as_posix()) + +from sphinx_exts import airflow_intersphinx # noqa: E402 + +PACKAGE = "apache-airflow-providers-ftp" + + [email protected] +def generated_path(tmp_path, monkeypatch): + monkeypatch.setattr(airflow_intersphinx, "GENERATED_PATH", tmp_path) + return tmp_path + + +def _touch(path: Path) -> Path: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(b"") + return path + + +class TestInventoryFor: + def test_prefers_the_html_inventory_built_in_this_run(self, generated_path): + build_dir = generated_path / "_build" / "docs" / PACKAGE / "stable" + html = _touch(build_dir / "objects.inv") + _touch(build_dir / f"output-spelling-results-{PACKAGE}" / "objects.inv") + _touch(generated_path / "_inventory_cache" / PACKAGE / "objects.inv") + + assert airflow_intersphinx._inventory_for(PACKAGE, versioned=True) == html + + def test_uses_the_spelling_inventory_when_only_a_spelling_build_ran(self, generated_path): + build_dir = generated_path / "_build" / "docs" / PACKAGE / "stable" + spelling = _touch(build_dir / f"output-spelling-results-{PACKAGE}" / "objects.inv") + _touch(generated_path / "_inventory_cache" / PACKAGE / "objects.inv") + + assert airflow_intersphinx._inventory_for(PACKAGE, versioned=True) == spelling + + def test_falls_back_to_the_downloaded_inventory(self, generated_path): + cached = generated_path / "_inventory_cache" / PACKAGE / "objects.inv" + + assert airflow_intersphinx._inventory_for(PACKAGE, versioned=True) == cached + + def test_non_versioned_packages_have_no_stable_directory(self, generated_path): + html = _touch(generated_path / "_build" / "docs" / "docker-stack" / "objects.inv") + + assert airflow_intersphinx._inventory_for("docker-stack", versioned=False) == html + + +class TestDumpInventoryAfterSpellingBuild: + @staticmethod + def _app(builder_name: str, outdir: Path) -> mock.Mock: + app = mock.Mock(spec=Sphinx) + app.builder = mock.Mock(spec=Builder) + app.builder.name = builder_name + app.outdir = outdir + app.env = mock.Mock(spec=BuildEnvironment) + return app + + @mock.patch.object(airflow_intersphinx.InventoryFile, "dump", autospec=True) + def test_writes_an_inventory_into_the_spelling_output_dir(self, mock_dump, tmp_path): + app = self._app("spelling", tmp_path) + + airflow_intersphinx._dump_inventory_after_spelling_build(app, None) + + mock_dump.assert_called_once() + filename, env, uri_builder = mock_dump.call_args.args + assert filename == (tmp_path / "objects.inv").as_posix() + assert env is app.env + assert uri_builder.get_target_uri("operators/index") == "operators/index.html" + + @pytest.mark.parametrize( + ("builder_name", "exception"), + [ + pytest.param("html", None, id="html-builder-writes-its-own"), + pytest.param("spelling", SphinxError("boom"), id="failed-build"), + ], + ) + @mock.patch.object(airflow_intersphinx.InventoryFile, "dump", autospec=True) + def test_skips_other_builders_and_failed_builds(self, mock_dump, tmp_path, builder_name, exception): + app = self._app(builder_name, tmp_path) + + airflow_intersphinx._dump_inventory_after_spelling_build(app, exception) + + mock_dump.assert_not_called()
