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

Pip compile check mode #1950

Merged
merged 2 commits into from
Oct 15, 2024
Merged

Conversation

gotmax23
Copy link
Collaborator

Support a custom --check flag to fail if pip-compile made any changes so
we can check that that lockfiles are in sync with the input (.in) files on all pushes and PRs.

Then, we can also remove the push trigger from the pip-compile workflows as discussed in #1920.

Support a custom --check flag to fail if pip-compile made any changes so
we can check that that lockfiles are in sync with the input (.in) files.
Now that we have the pip-compile-check job, this is redundant. PR checks
will fail if lockfiles are out-of-sync, so there's no need to run the
complete pip-compile workflow that performs a full update of the
dependencies. Complete updates are still performed on a weekly basis.
@gotmax23 gotmax23 marked this pull request as ready for review September 26, 2024 15:30
@gotmax23 gotmax23 added no_backport This PR should not be backported. devel only. tooling This PR affects tooling (CI, pr_labeler, noxfile, linters, etc.) but not the docs builds themselves. merge_commit Use "Create a merge commit" when applying this PR labels Sep 26, 2024
@gotmax23 gotmax23 requested a review from oraNod September 26, 2024 15:31
noxfile.py Show resolved Hide resolved
Copy link
Contributor

@oraNod oraNod left a comment

Choose a reason for hiding this comment

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

Thanks @gotmax23 🚀

@oraNod oraNod merged commit ac88861 into ansible:devel Oct 15, 2024
11 checks passed
@felixfontein
Copy link
Collaborator

@gotmax23 @oraNod this PR introduced a problem: the new pip-compile session is also run for every single PR and push, and not just scheduled. That means that if a user creates a PR when the deps are outdated, CI for that PR will fail. If someone merges a PR while the deps are outdated, CI for devel fails.

I don't think it should be like that. I think it would be best if pip-compile would only run on schedule or manual trigger. I guess on push is debatable (CI for devel will fail until the deps are updated, even if the new deps totally break the docs build and it needs some time and work to get things fixed), but it definitely should not run for regular docs PRs.

@gotmax23
Copy link
Collaborator Author

The check doesn't care if the deps are outdated. It runs pip-compile without --upgrade to ensure that the lockfiles are in sync with the input files. So if, e.g., bar, was added to requirements.in but requirements.txt didn't contain a pin for bar, this would fail, but if, e.g., foo was already pinned in requirements.txt but at an older version, foo would be left alone.

@gotmax23
Copy link
Collaborator Author

Okay, I see #2076. When I run nox -e pip-compile -- --check locally, I get this diff:

diff --git a/tests/requirements-relaxed.txt b/tests/requirements-relaxed.txt
index 51e21ed1d9..bd1d1c0062 100644
--- a/tests/requirements-relaxed.txt
+++ b/tests/requirements-relaxed.txt
@@ -116,7 +116,7 @@ resolvelib==1.0.1
     # via -r tests/requirements-relaxed.in
 rstcheck==3.5.0
     # via
-    #   -c tests/constraints-base.in
+    #   -c /home/gotmax/dev/ansible-documentation/tests/constraints-base.in
     #   -r tests/requirements-relaxed.in
     #   antsibull-changelog
     #   antsibull-docs
@@ -149,7 +149,7 @@ sphinx-notfound-page==1.0.4
     # via -r tests/requirements-relaxed.in
 sphinx-rtd-theme==2.0.0
     # via
-    #   -c tests/constraints-base.in
+    #   -c /home/gotmax/dev/ansible-documentation/tests/constraints-base.in
     #   sphinx-ansible-theme
 sphinxcontrib-applehelp==2.0.0
     # via sphinx
diff --git a/tests/requirements.txt b/tests/requirements.txt
index 8286a2188e..7cb0909baa 100644
--- a/tests/requirements.txt
+++ b/tests/requirements.txt
@@ -30,8 +30,8 @@ antsibull-core==3.3.0
     # via antsibull-docs
 antsibull-docs==2.15.0
     # via
-    #   -c tests/constraints.in
-    #   -r tests/requirements-relaxed.in
+    #   -c /home/gotmax/dev/ansible-documentation/tests/constraints.in
+    #   -r /home/gotmax/dev/ansible-documentation/tests/requirements-relaxed.in
 antsibull-docs-parser==1.1.0
     # via antsibull-docs
 antsibull-docutils==1.0.0
@@ -77,7 +77,7 @@ imagesize==1.4.1
     # via sphinx
 jinja2==3.1.4
     # via
-    #   -r tests/requirements-relaxed.in
+    #   -r /home/gotmax/dev/ansible-documentation/tests/requirements-relaxed.in
     #   antsibull-docs
     #   sphinx
 markupsafe==2.1.5
@@ -109,17 +109,17 @@ pyproject-hooks==1.1.0
     # via build
 pyyaml==6.0.2
     # via
-    #   -r tests/requirements-relaxed.in
+    #   -r /home/gotmax/dev/ansible-documentation/tests/requirements-relaxed.in
     #   antsibull-docs
     #   antsibull-fileutils
 requests==2.32.3
     # via sphinx
 resolvelib==1.0.1
-    # via -r tests/requirements-relaxed.in
+    # via -r /home/gotmax/dev/ansible-documentation/tests/requirements-relaxed.in
 rstcheck==5.0.0
     # via
-    #   -c tests/constraints-base.in
-    #   -r tests/requirements-relaxed.in
+    #   -c /home/gotmax/dev/ansible-documentation/tests/constraints-base.in
+    #   -r /home/gotmax/dev/ansible-documentation/tests/requirements-relaxed.in
     #   antsibull-changelog
     #   antsibull-docs
 semantic-version==2.10.0
@@ -133,8 +133,8 @@ snowballstemmer==2.2.0
     # via sphinx
 sphinx==7.2.5
     # via
-    #   -c tests/constraints.in
-    #   -r tests/requirements-relaxed.in
+    #   -c /home/gotmax/dev/ansible-documentation/tests/constraints.in
+    #   -r /home/gotmax/dev/ansible-documentation/tests/requirements-relaxed.in
     #   antsibull-docs
     #   sphinx-ansible-theme
     #   sphinx-copybutton
@@ -143,16 +143,16 @@ sphinx==7.2.5
     #   sphinx-rtd-theme
     #   sphinxcontrib-jquery
 sphinx-ansible-theme==0.10.3
-    # via -r tests/requirements-relaxed.in
+    # via -r /home/gotmax/dev/ansible-documentation/tests/requirements-relaxed.in
 sphinx-copybutton==0.5.2
-    # via -r tests/requirements-relaxed.in
+    # via -r /home/gotmax/dev/ansible-documentation/tests/requirements-relaxed.in
 sphinx-intl==2.2.0
-    # via -r tests/requirements-relaxed.in
+    # via -r /home/gotmax/dev/ansible-documentation/tests/requirements-relaxed.in
 sphinx-notfound-page==1.0.4
-    # via -r tests/requirements-relaxed.in
+    # via -r /home/gotmax/dev/ansible-documentation/tests/requirements-relaxed.in
 sphinx-rtd-theme==2.0.0
     # via
-    #   -c tests/constraints-base.in
+    #   -c /home/gotmax/dev/ansible-documentation/tests/constraints-base.in
     #   sphinx-ansible-theme
 sphinxcontrib-applehelp==2.0.0
     # via sphinx
diff --git a/tests/typing.txt b/tests/typing.txt
index 421cca0454..278b880e33 100644
--- a/tests/typing.txt
+++ b/tests/typing.txt
@@ -19,7 +19,7 @@ click==8.1.7
     #   typer
     #   typer-slim
 codeowners==0.7.0
-    # via -r tests/../hacking/pr_labeler/requirements.txt
+    # via -r /home/gotmax/dev/ansible-documentation/hacking/pr_labeler/requirements.txt
 colorlog==6.8.2
     # via nox
 cryptography==43.0.1
@@ -33,11 +33,11 @@ filelock==3.16.0
 gitdb==4.0.11
     # via gitpython
 gitpython==3.1.43
-    # via -r tests/tag.in
+    # via -r /home/gotmax/dev/ansible-documentation/tests/tag.in
 idna==3.9
     # via requests
 jinja2==3.1.4
-    # via -r tests/../hacking/pr_labeler/requirements.txt
+    # via -r /home/gotmax/dev/ansible-documentation/hacking/pr_labeler/requirements.txt
 markdown-it-py==3.0.0
     # via rich
 markupsafe==2.1.5
@@ -52,14 +52,14 @@ nox==2024.4.15
     # via -r tests/typing.in
 packaging==24.1
     # via
-    #   -r tests/tag.in
+    #   -r /home/gotmax/dev/ansible-documentation/tests/tag.in
     #   nox
 platformdirs==4.3.3
     # via virtualenv
 pycparser==2.22
     # via cffi
 pygithub==2.4.0
-    # via -r tests/../hacking/pr_labeler/requirements.txt
+    # via -r /home/gotmax/dev/ansible-documentation/hacking/pr_labeler/requirements.txt
 pygments==2.18.0
     # via rich
 pyjwt==2.9.0
@@ -75,9 +75,9 @@ shellingham==1.5.4
 smmap==5.0.1
     # via gitdb
 typer==0.12.5
-    # via -r tests/tag.in
+    # via -r /home/gotmax/dev/ansible-documentation/tests/tag.in
 typer-slim==0.12.5
-    # via -r tests/../hacking/pr_labeler/requirements.txt
+    # via -r /home/gotmax/dev/ansible-documentation/hacking/pr_labeler/requirements.txt
 typing-extensions==4.12.2
     # via
     #   codeowners

So it seems some change was made in upstream pip-compile that breaks the way these comments (I think pip-compile calls them annotations) work. Potential solutions are:

  1. Pin pip-compile. This is something we should probably do anyways.
  2. Disable annotations feature. If we do this, pinning is still worthwhile to prevent other formatting changes from causing unrelated failures in PRs.
  3. Use this as an opportunity/excuse to switch to uv pip-compile and also pin the uv version used (see ci: switch pip-compile jobs to use uv pip compile  #1755).

@felixfontein, @oraNod, what do you think?

@gotmax23
Copy link
Collaborator Author

So it seems some change was made in upstream pip-compile that breaks the way these comments (I think pip-compile calls them annotations) work.

See jazzband/pip-tools#2131.

@oraNod
Copy link
Contributor

oraNod commented Oct 29, 2024

hey @gotmax23 and @felixfontein I'm away from my laptop for the next couple days. out in the wild with my kids. 🏕️

I think pinning pip-compile sounds better than disabling annotations. switching to uv is probably a good idea but I'm not sure I'm well versed enough to justify that decision. I know it's the cool new thing and is a lot more performant but I haven't really used it beyond just playing around. if you agree that is the better option though, I'm on board. cheers

gotmax23 added a commit to gotmax23/ansible-documentation that referenced this pull request Oct 29, 2024
This switches the nox pip-compile session and lockfiles to use uv pip
compile (written in Rust and much faster) instead of pip-compile from
pip-tools for our dependency update jobs.

As a side effect, this addresses the issue brought up in
ansible#1950 (comment),
as we're no longer using pip-compile that can break anytime pip changes
the private/internal APIs that pip-tools relies on.

I re-generated the lockfiles with `--no-upgrade` to avoid extraneous
changes in this PR.
The only additions are the formatting of comments in the lockfile and
colorama.
colorama was added as a result of `uv pip-compile`'s `--universal` flag
which creates "universal" lockfiles that include dependencies of other
platforms, architectures, and Python versions than those of the system
used to generate the lockfile.
`--universal` should create more consistent results for contributors who
use other systems
(e.g., Mac OS or a newer Python version than the one used in CI).

Fixes: ansible#1755
@gotmax23
Copy link
Collaborator Author

#2084 implements solution 3, but I'll try to come up with another, more temporary PR to fix the immediate issue.

gotmax23 added a commit to gotmax23/ansible-documentation that referenced this pull request Oct 29, 2024
@gotmax23
Copy link
Collaborator Author

but I'll try to come up with another, more temporary PR to fix the immediate issue.

#2085 does that. Quick reviews would be appreciated to fix our broken CI.

felixfontein pushed a commit that referenced this pull request Oct 29, 2024
felixfontein pushed a commit that referenced this pull request Oct 29, 2024
samccann pushed a commit that referenced this pull request Oct 29, 2024
patchback bot pushed a commit that referenced this pull request Nov 6, 2024
gotmax23 added a commit that referenced this pull request Nov 6, 2024
patchback bot pushed a commit that referenced this pull request Nov 6, 2024
gotmax23 added a commit that referenced this pull request Nov 6, 2024
gotmax23 added a commit to gotmax23/ansible-documentation that referenced this pull request Nov 23, 2024
This switches the nox pip-compile session and lockfiles to use uv pip
compile (written in Rust and much faster) instead of pip-compile from
pip-tools for our dependency update jobs.

As a side effect, this addresses the issue brought up in
ansible#1950 (comment),
as we're no longer using pip-compile that can break anytime pip changes
the private/internal APIs that pip-tools relies on.

I re-generated the lockfiles with `--no-upgrade` to avoid extraneous
changes in this PR.
The only additions are the formatting of comments in the lockfile and
colorama.
colorama was added as a result of `uv pip-compile`'s `--universal` flag
which creates "universal" lockfiles that include dependencies of other
platforms, architectures, and Python versions than those of the system
used to generate the lockfile.
`--universal` should create more consistent results for contributors who
use other systems
(e.g., Mac OS or a newer Python version than the one used in CI).

Fixes: ansible#1755
gotmax23 added a commit to gotmax23/ansible-documentation that referenced this pull request Nov 23, 2024
This switches the nox pip-compile session and lockfiles to use uv pip
compile (written in Rust and much faster) instead of pip-compile from
pip-tools for our dependency update jobs.

As a side effect, this addresses the issue brought up in
ansible#1950 (comment),
as we're no longer using pip-compile that can break anytime pip changes
the private/internal APIs that pip-tools relies on.

I re-generated the lockfiles with `--no-upgrade` to avoid extraneous
changes in this PR.
The only additions are the formatting of comments in the lockfile and
colorama.
colorama was added as a result of `uv pip-compile`'s `--universal` flag
which creates "universal" lockfiles that include dependencies of other
platforms, architectures, and Python versions than those of the system
used to generate the lockfile.
`--universal` should create more consistent results for contributors who
use other systems
(e.g., Mac OS or a newer Python version than the one used in CI).

Fixes: ansible#1755
oraNod pushed a commit that referenced this pull request Nov 25, 2024
* nox pip-compile: support check mode

Support a custom --check flag to fail if pip-compile made any changes so
we can check that that lockfiles are in sync with the input (.in) files.

* ci: remove push trigger for pip-compile workflows

Now that we have the pip-compile-check job, this is redundant. PR checks
will fail if lockfiles are out-of-sync, so there's no need to run the
complete pip-compile workflow that performs a full update of the
dependencies. Complete updates are still performed on a weekly basis.
gotmax23 added a commit to gotmax23/ansible-documentation that referenced this pull request Dec 3, 2024
This switches the nox pip-compile session and lockfiles to use uv pip
compile (written in Rust and much faster) instead of pip-compile from
pip-tools for our dependency update jobs.

As a side effect, this addresses the issue brought up in
ansible#1950 (comment),
as we're no longer using pip-compile that can break anytime pip changes
the private/internal APIs that pip-tools relies on.

I re-generated the lockfiles with `--no-upgrade` to avoid extraneous
changes in this PR.
The only additions are the formatting of comments in the lockfile and
colorama.
colorama was added as a result of `uv pip-compile`'s `--universal` flag
which creates "universal" lockfiles that include dependencies of other
platforms, architectures, and Python versions than those of the system
used to generate the lockfile.
`--universal` should create more consistent results for contributors who
use other systems
(e.g., Mac OS or a newer Python version than the one used in CI).

Fixes: ansible#1755
oraNod pushed a commit that referenced this pull request Dec 4, 2024
* tests: switch to uv pip-compile for lockfiles

This switches the nox pip-compile session and lockfiles to use uv pip
compile (written in Rust and much faster) instead of pip-compile from
pip-tools for our dependency update jobs.

As a side effect, this addresses the issue brought up in
#1950 (comment),
as we're no longer using pip-compile that can break anytime pip changes
the private/internal APIs that pip-tools relies on.

I re-generated the lockfiles with `--no-upgrade` to avoid extraneous
changes in this PR.
The only additions are the formatting of comments in the lockfile and
colorama.
colorama was added as a result of `uv pip-compile`'s `--universal` flag
which creates "universal" lockfiles that include dependencies of other
platforms, architectures, and Python versions than those of the system
used to generate the lockfile.
`--universal` should create more consistent results for contributors who
use other systems
(e.g., Mac OS or a newer Python version than the one used in CI).

Fixes: #1755

* nox pip_compile: improve code comments

Suggested-by: Don Naro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge_commit Use "Create a merge commit" when applying this PR no_backport This PR should not be backported. devel only. tooling This PR affects tooling (CI, pr_labeler, noxfile, linters, etc.) but not the docs builds themselves.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants