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

Upgrade sphinx and related dependencies #45563

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

shahar1
Copy link
Contributor

@shahar1 shahar1 commented Jan 10, 2025

closes: #31963
related: #39449

This PR update the following dependencies:

  • sphinx to v7+ (there's already v8, but it is supported only for Python 3.10)
  • docutils to v0.21+ (haven't encountered any <section> tags as mentioned in the deleted inline comment)
  • sphinx-autoapi to v3+ and astroid to v3+ (possible now that sphinx is v7+)
  • sphinxcontrib-serializinghtml to v1.1.5+ (pinning it causes dependency conflicts)

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@shahar1 shahar1 added area:dependencies Issues related to dependencies problems backport-to-v2-10-test Mark PR with this label to backport to v2-10-test branch labels Jan 10, 2025
@shahar1 shahar1 marked this pull request as draft January 10, 2025 18:57
@shahar1
Copy link
Contributor Author

shahar1 commented Jan 10, 2025

Drafting, weirdly on v2-10-test it worked fine but here there are errors

@potiuk
Copy link
Member

potiuk commented Jan 10, 2025

one more thing - last time when we tried to upgrade sphinx and docutils, the documentation looked ....weird .... lots of extra whitespace

@shahar1
Copy link
Contributor Author

shahar1 commented Jan 11, 2025

Apparently it is a quite a tough task, here are some insights:

  • A requirement for upgrading Sphinx to v7 is to merge [BREAKING CHANGE] Update link tag for compatibility with Sphinx v7 airflow-site#1103 and create a new version for sphinx_airflow_theme (0.1.0). I struggled with mounting the changes locally, so for now, I made a workaround by injecting the contents of the file that was modified via Dockerfile.ci (created an issue for anyone who wants to tackle it in the future).

  • When running only the build docs workflow, locally and on GitHub Actions, using breeze build-docs apache-airflow docker-stack --docs-only --clean-build, the build succeeds without any errors (see GitHub Action workflow). When running the docs server locally, the extra spaces issue indeed persists - we need to find out how to modify the related CSS (no success with it so far).
    image

  • When running only the spellchecks workflow, locally and on GitHub Actions, using breeze build-docs apache-airflow docker-stack --spellchecks-only --clean-build (see raw logs - BIG FILE warning) - we get a lot of weird errors. If you take a look at the logs, it often fails when parsing from /opt/airflow/providers/src/airflow/providers/amazon/aws/hooks/ssm.py::SsmHook:: get_parameter_value. Edit: it happened due to missing words in the spellcheck wordlist (see Update spelling wordlist #45579) - now it should be fine.

  • When running the workflow for building docs and spelling altogether (breeze build-docs apache-airflow docker-stack --clean-build), the build fails due to a large number of implicit errors (which might be related to the failure of the spelling checks?).

  • The MyPy workflow fails, but it doesn't seem too bad - in the worst case we just ignore it.

I'll be happy for some help in solving some of the riddles above, it will gets us closer to upgrading to Sphinx 7 :)

@potiuk
Copy link
Member

potiuk commented Jan 11, 2025

#protm

@shahar1 shahar1 marked this pull request as ready for review January 11, 2025 21:57
@shahar1 shahar1 requested a review from ashb as a code owner January 11, 2025 21:57
@@ -1355,6 +1355,402 @@ ENV PATH="/files/bin/:/opt/airflow/scripts/in_container/bin/:${PATH}" \
# so we can go back to the default link mode being hardlink.
Copy link
Contributor Author

@shahar1 shahar1 Jan 11, 2025

Choose a reason for hiding this comment

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

This change should be removed before merging

# By adding a lot of whitespace separation. This limit can be lifted when we update our doc to handle
# <section> tags for sections
"docutils<0.17,>=0.16",
"docutils>=0.21",
"sphinx-airflow-theme>=0.0.12",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before merging, we should create a new version for sphinx-airflow-theme (0.1.0) after merging apache/airflow-site#1103

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dependencies Issues related to dependencies problems backport-to-v2-10-test Mark PR with this label to backport to v2-10-test branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade Sphinx to version 7 to improve documentation errors
3 participants