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

Docs build: Add upper bounds to transitive dependencies #103860

Merged
merged 4 commits into from
Apr 26, 2023

Conversation

AlexWaygood
Copy link
Member

Our immediate requirements for the docs build are already pinned pretty tightly in our Doc/requirements.txt file. However, our transitive dependencies are currently entirely unpinned. This means our CI can be unexpectedly broken by a new release of a transitive dependency, as recently happened when Pygments==2.15.0 was released: see #103421.

Previously it wasn't so much of a big deal if the Docs build got broken briefly. However, with the recent move to use GitHub automerge instead of @miss-islington's merging capabilities, it's now important that we make the Docs workflow a "required" check, so that PRs with broken ReST syntax don't accidentally get automerged into the main branch. Making the docs workflow a "required" check means it is more important to make sure that the Docs build is very resilient.

This PR adds upper bounds to various transitive dependencies by using a constraints.txt file.

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @AlexWaygood . We should presumably backport this as well, so backport PRs that are automerged can be treated similarly

@AlexWaygood
Copy link
Member Author

We should presumably backport this as well, so backport PRs that are automerged can be treated similarly

Yes, that SGTM!

Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

Seems a good idea in principle -- my questions would be how we ensure that new transitive dependencies are always added (including dependencies of dependencies, etc), and when we consider it appropriate to update the upper-bounds (security certainly, new feature releases -- maybe?).

A

Doc/constraints.txt Show resolved Hide resolved
Doc/constraints.txt Outdated Show resolved Hide resolved
@AlexWaygood AlexWaygood requested a review from AA-Turner April 26, 2023 00:20
@AlexWaygood
Copy link
Member Author

AlexWaygood commented Apr 26, 2023

my questions would be how we ensure that new transitive dependencies are always added (including dependencies of dependencies, etc), and when we consider it appropriate to update the upper-bounds (security certainly, new feature releases -- maybe?).

I don't have fully formed answers to these questions, but I agree that we will need to work these things out :)

What's the current process for updating our Doc/requirements.txt file? I'd imagine that the constraints.txt file would generally only get updated when the requirements.txt file is updated -- which seems fine to me?

@AA-Turner
Copy link
Member

What's the current process for updating our Doc/requirements.txt file?

@JulienPalard is probably the man for the 'policy' answer here, though my guess is ad-hoc PRs as needed (as with many other things!). In which case pragmatism seems the best approach as you suggest -- hopefully reviewers notice / remember the constraints file, but fundamentally if we make a mistake it's just a quick PR to fix!

Thanks for the quick response -- more appreciated than usual as it's past midnight British time!

A

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Apr 26, 2023

At this point, maybe worth just using something like pip-tools (pip-compile) to just generate a pseudo-lockfile and use that (only) in the CIs, and schedule periodic (e.g. monthly) CI runs to update it? That's a lot more maintainable than just relying on people manually maintaining (somehow...) at irregular intervals.

Also, is this going to get picked up by the build server infra?

@bedevere-bot

This comment was marked as resolved.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Apr 26, 2023
@JelleZijlstra

This comment was marked as resolved.

@AlexWaygood
Copy link
Member Author

At this point, maybe worth just using something like pip-tools (pip-compile) to just generate a pseudo-lockfile and use that (only) in the CIs, and schedule periodic (e.g. monthly) CI runs to update it? That's a lot more maintainable than just relying on people manually maintaining (somehow...) at irregular intervals.

I'd be okay with that. I also feel like the maintainability here isn't really going to be a problem though. We'll probably only need to update it whenever requirements.txt is updated, which won't be very often. I'd advocate for merging something like this now, and maybe investigating a more elaborate solution using pip-tools and a periodic GitHub Action job as a followup issue.

Also, is this going to get picked up by the build server infra?

No idea, I don't know anything about the build server infra 😃 Who do I ask about that?

@AA-Turner
Copy link
Member

AA-Turner commented Apr 26, 2023

Also, is this going to get picked up by the build server infra?

No idea, I don't know anything about the build server infra 😃 Who do I ask about that?

It will be! See docsbuild-scripts:

https://github.com/python/docsbuild-scripts/blob/main/build_docs.py#L105

and

https://github.com/python/docsbuild-scripts/blob/main/build_docs.py#L123

A

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

LGTM. I would go ahead and merge and we can adjust as needed. @pradyunsg may have some additional insights on dependencies.

@AlexWaygood
Copy link
Member Author

Great, thanks everybody! Feels like we have consensus that this is a good idea and that this is good enough for now, so I'll go ahead and merge. Followup PRs to improve our workflow around this are welcome :)

@AlexWaygood AlexWaygood merged commit 81cf94c into python:main Apr 26, 2023
@miss-islington
Copy link
Contributor

Thanks @AlexWaygood for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@AlexWaygood AlexWaygood deleted the docs-constraints branch April 26, 2023 17:07
@miss-islington
Copy link
Contributor

Sorry @AlexWaygood, I had trouble checking out the 3.11 backport branch.
Please retry by removing and re-adding the "needs backport to 3.11" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 81cf94c4426b3bb949be8a0bb26ae0adccdbc88c 3.11

@AlexWaygood AlexWaygood added needs backport to 3.11 only security fixes and removed needs backport to 3.11 only security fixes labels Apr 26, 2023
@miss-islington
Copy link
Contributor

Thanks @AlexWaygood for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-103887 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Apr 26, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 26, 2023
)

(cherry picked from commit 81cf94c)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
AlexWaygood added a commit that referenced this pull request Apr 26, 2023
…3860) (#103887)

Docs build: Add upper bounds to transitive dependencies (GH-103860)
(cherry picked from commit 81cf94c)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
itamaro pushed a commit to itamaro/cpython that referenced this pull request Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip issue skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants