Update lower boundary in Python pip pyproject when bumping versions #6631 #10060

kbukum1 · 2024-06-22T03:22:19Z

What are you trying to accomplish?

The goal of this PR is to correct the behavior of the versioning strategy in Dependabot for Python dependencies. Specifically, when using the "increase" strategy.

Why: The current behavior was widening the range (e.g., from >=8,<9 to >=8,<10), which is not the intended behavior. Instead, it should keep lower bound version as the new latest version and just move forward the upper boundary version (e.g., from >=8,<9 to >=9,<10) as mentioned Documentation for Versioning Strategies

Issues this affects or fixes:

Addresses the issue reported in The increase strategy should move ranges forward, not widen them #6631.
First noticed at Python: dependabot refuses to bump requirements.txt for non-poetry pyproject.toml projects #6625 (comment).

Anything you want to highlight for special attention from reviewers?

The changes involve modifying the requirements for the pip ecosystem to ensure correct behavior for the "increase" strategy. Although the issue suggests above major versioning (e.g. >=8,<9), our "increase" strategy mandates that the lower bound is always to be the latest version (e.g. >=8.0.1,<9). This approach aligns with the guidelines provided in the Dependabot documentation. Therefore, the changes have been made accordingly.

How will you know you've accomplished your goal?

Updated smoke tests should run successfully.
Update Smoke Tests for "Increase" Versioning Strategy Adjustment in Python Dependencies smoke-tests#212
Testing on https://github.com/dsp-testing/dependabot-updates-issues-6631 should create PR according to the changes

Checklist

I have run the complete test suite to ensure all tests and linters pass.
I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
I have written clear and descriptive commit messages.
I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
I have ensured that the code is well-documented and easy to understand.

kbukum1 · 2024-06-26T17:23:22Z

Smoke tests are updated: https://github.com/dependabot/smoke-tests/pull/212/files
So the PR needs to be merged to run smoke tests successfully.

abdulapopoola · 2024-06-27T00:54:52Z

Tagging @jeffwidman and @deivid-rodriguez for PR reviews too

python/lib/dependabot/python/update_checker/requirements_updater.rb

honeyankit · 2024-06-27T22:19:14Z

@kbukum1 The python CI test is failing with your last commit, so I have revoked the approval.

kbukum1 · 2024-06-27T22:29:21Z

@kbukum1 The python CI test is failing with your last commit, so I have revoked the approval.

Fixing it

…rease strategy).

kbukum1 · 2024-06-27T23:46:11Z

ers w

fixed

deivid-rodriguez

Hei! I did not review the implementation, but there's some things that could be improved in my opinion in the new test outcomes.

deivid-rodriguez · 2024-06-28T07:12:44Z

python/spec/dependabot/python/update_checker/requirements_updater_spec.rb


              context "when requirement version has more digits than the new version" do
                let(:requirement_txt_req_string) { "<=1.9.2,>=1.9" }
                let(:latest_resolvable_version) { "1.10" }

-                its([:requirement]) { is_expected.to eq(">=1.9,<=1.10") }
+                its([:requirement]) { is_expected.to eq("<=1.10,>=1.10") }


In this case, there's no clear solution, but <=1.10,>=1.10 feels not right? Either one of:

<=1.11,>=1.10.

<=1.10.2,>=1.10

:unfixable

I'd probably leave this one alone as :unfixable since it's unclear what the user wants in the case.

deivid-rodriguez · 2024-06-28T07:18:58Z

python/spec/dependabot/python/update_checker/requirements_updater_spec.rb

@@ -370,7 +388,7 @@
          context "when the requirement version is too high" do
            let(:setup_cfg_req_string) { ">=2.0.0" }

-            its([:requirement]) { is_expected.to eq(:unfixable) }
+            its([:requirement]) { is_expected.to eq(">=1.5.0") }


I like the previous outcome better here. If the latest resolvable version is 1.5.0, and user has specified >=2.0.0 in their manifest, I think either:

User setup/manifest is wrong.

Dependabot run into a bug when figuring out the latest resolvable version.

Either way, it seems best to leave this kind of ill defined case alone?

Also, the new solution is actually widening the requirement, not increasing it, which seems completely against the spirit of this PR?

deivid-rodriguez · 2024-06-28T07:20:23Z

python/spec/dependabot/python/update_checker/requirements_updater_spec.rb

@@ -381,7 +399,7 @@
            context "when needing an update" do
              let(:setup_cfg_req_string) { ">=1.3.0, <1.5" }

-              its([:requirement]) { is_expected.to eq(">=1.3.0,<1.6") }
+              its([:requirement]) { is_expected.to eq(">=1.5.0,<1.6") }


Same as the previous comment on the similar case on requirement.txt files.

deivid-rodriguez · 2024-06-28T07:23:42Z

python/spec/dependabot/python/update_checker/requirements_updater_spec.rb

+
+            its([:requirement]) { is_expected.to eq(:unfixable) }
+          end
+        end


The above block of specs makes sense to me 👍.

deivid-rodriguez · 2024-06-28T07:26:00Z

python/spec/dependabot/python/update_checker/requirements_updater_spec.rb

@@ -129,7 +147,7 @@
          context "when requirement version is too high" do
            let(:requirement_txt_req_string) { ">=2.0.0" }

-            its([:requirement]) { is_expected.to eq(:unfixable) }
+            its([:requirement]) { is_expected.to eq(">=1.5.0") }


I like the previous outcome better here. If the latest resolvable version is 1.5.0, and user has specified >=2.0.0 in their manifest, I think either:

User setup/manifest is wrong.

Dependabot run into a bug when figuring out the latest resolvable version.

Either way, it seems best to leave this kind of ill defined case alone?

Also, the new solution is actually widening the requirement, not increasing it, which seems completely against the spirit of this PR?

The new solution actually it is not widening it. It is setting the lower bound as the latest version and the upper bound is still following the last version by increasing. As it is mentioned in the document tried to follow the typical scenario.

Always increase the minimum version requirement to match the new version. If a range already exists, typically this only increases the lower bound.

If I'm understanding this test case correctly, the previous requirement is >= 2.0.0, correct? If Dependabot sets the new range to be >= 1.5.0, it's actually decreasing the lower bound and thus widening the requirement.

This seems like an ill-defined case to me. Why would the existing range be >= 2.0.0 if the latest version is 1.5.0? That's why it seemed reasonable to not mess with this case and call it "unfixable".

That said, I just came up with a potential case when this could happen in real life: If latest version 2.0.0 is yanked, then that could make 1.5.0 the "new" latest version and make a case like this possible.

Previously Dependabot would avoid messing up with this case, and now it's downgrading the version to 1.5.0. I guess it's not as bad after all!

I am strongly of the opinion that dependabot should never lower a >= constraint. When such a constraint is used, I believe it means one of two things:

We're tracking the latest version: the user should pick the increase strategy.

We're declaring the range of all compatible versions and we know that lower versions are not compatible (e.g. we're using a feature that was introduced in 2.0): user should pick widen strategy and dependabot should not touch lower bound.

I realize I got the context wrong: we're talking about increase here. I still think a user intervention would be more appropriate here, but I am not as strongly invested as for the widen case.

Yeah, I do think there could be all sort of expectations here. I guess it could also happen that someone using widen has changed their code to support 2.0 version of some library and bumped their lower bound requirement to >=2.0, then 2.0 is abruptly removed and user may appreciated that Dependabot "rolls back" their requirement change and let them know through a PR they need to revert code changes too to get back on track.

Also, strictly speaking, for the widen strategy, Dependabot is also doing exactly what's advertised right? Namely, "widen the range to accommodate for the latest version". Just that in this case, the latest version is not newer than the upper bound, but older than the lower bound.

In any case, I think this is exactly why I advocate for "leave this kind of case alone", because different users will have different expectations, or even the same user may want different things depending on the library.

You do make a good point, this is a case where it's difficult to find a universal approach to fit everyone's use case. Your suggestion to leave it alone sounds sensible to me (biased though I am because that is what I've been advocating regardless).

When I implemented this, my thoughts were as follows:

In real-world scenarios, this occurs when no range was specified before, when a higher version is yanked and necessitates a downgrade to a previous release, or when the strategy wasn’t set to increase when a lower bound is added. In such cases, Dependabot can adjust the lower bound accordingly.

For the increase strategy, if a lower constraint is defined to ensure compatibility, the lower bound should not be higher than the latest version since the latest version is functional for the app or library. Otherwise, it risks excluding versions that work correctly, potentially breaking the app.

I would appreciate your thoughts on this.

I see what you're saying, but I still feel like it's a risky assumption to make for the widen strategy. For example, consider the case where a library detects a breaking bug in their latest release, so they yank it and then release a patched one soon after. If they do it in that order, dependabot might run in the window where the broken one was yanked but the new one not yet released. It would then change the lower bound, while it wouldn't have taken action at all if it had run a few minutes later.

If the library does not release a patched version (which is quite rare as far as I know), then action will indeed be required. But since this is such a rare occurrence, and lowering the constraint might not always be desired, I feel like it's safer to leave this as a problem for the user to solve.

That said, I'm just a bystander here, so I won't try to push my view any further than this.

I agree with @sanderr here. I think Dependabot should create update PRs that work for everyone, or when that's not the case, at least allow the behavior to be configurable. This seems such an edge case that feels best to not try be too smart and leave it for the user to fix manually.

deivid-rodriguez · 2024-06-28T07:28:49Z

python/spec/dependabot/python/update_checker/requirements_updater_spec.rb

@@ -504,7 +552,7 @@
              context "when the requirement version is too high" do
                let(:pyproject_req_string) { ">=2.0.0" }

-                its([:requirement]) { is_expected.to eq(:unfixable) }
+                its([:requirement]) { is_expected.to eq(">=1.5.0") }


Same as the previous comment on the similar case on requirement.txt files.

deivid-rodriguez · 2024-06-28T07:28:53Z

python/spec/dependabot/python/update_checker/requirements_updater_spec.rb

@@ -521,7 +569,7 @@
                context "when needing an update" do
                  let(:pyproject_req_string) { ">=1.3.0, <1.5" }

-                  its([:requirement]) { is_expected.to eq(">=1.3.0,<1.6") }
+                  its([:requirement]) { is_expected.to eq(">=1.5.0,<1.6") }


Same as the previous comment on the similar case on requirement.txt files.

deivid-rodriguez · 2024-06-28T07:29:23Z

python/spec/dependabot/python/update_checker/requirements_updater_spec.rb

+
+              its([:requirement]) { is_expected.to eq(:unfixable) }
+            end
+          end


The above block of specs makes sense to me 👍.

deivid-rodriguez · 2024-06-28T07:29:39Z

python/spec/dependabot/python/update_checker/requirements_updater_spec.rb

@@ -650,7 +728,7 @@
            context "when the requirement is too high" do
              let(:pyproject_req_string) { ">=2.0.0" }

-              its([:requirement]) { is_expected.to eq(:unfixable) }
+              its([:requirement]) { is_expected.to eq(">=1.5.0") }


Same as the previous comment on the similar case on requirement.txt files.

deivid-rodriguez · 2024-06-28T07:29:42Z

python/spec/dependabot/python/update_checker/requirements_updater_spec.rb

@@ -661,7 +739,7 @@
              context "when needing an update" do
                let(:pyproject_req_string) { ">=1.3.0, <1.5" }

-                its([:requirement]) { is_expected.to eq(">=1.3.0,<1.6") }
+                its([:requirement]) { is_expected.to eq(">=1.5.0,<1.6") }


Same as the previous comment on the similar case on requirement.txt files.

deivid-rodriguez · 2024-06-28T07:49:18Z

Also, I think it's fine to treat this a bug and change how Dependabot updates manifests here.

However, as pointed out by @sanderr here, it'd be nice to enable an explicit widen strategy too to allow keeping the same behavior as before this PR, so that people considering this bug a feature, can still get the behavior they want :)

jeffwidman · 2024-06-28T20:02:22Z

Thanks @deivid-rodriguez!

I was going to take a look at this--just running late--but @kbukum1 I'll wait until you put up another follow-on PR addressing some of this feedback and then take a deeper look myself.

kbukum1 · 2024-07-02T17:12:16Z

I am currently preparing a doc for the current increase and widening versioning strategies. After going through reviews here I plan to create a new issue that is going to be used for finalizing both versioning strategies. Is that sound ok @deivid-rodriguez , @jeffwidman

…0194) * Revert "Added fix for lower pyproject low boundry issue for bump version (increase strategy)."

kbukum1 · 2024-07-11T17:07:20Z

The changes of this PR are reverted in #10194. The versioning strategy needs to be re-planned, and a solution will be implemented according to the new plan.

github-actions bot added the L: python label Jun 22, 2024

kbukum1 self-assigned this Jun 24, 2024

kbukum1 requested review from abdulapopoola and honeyankit June 24, 2024 17:45

kbukum1 mentioned this pull request Jun 25, 2024

Update Smoke Tests for "Increase" Versioning Strategy Adjustment in Python Dependencies dependabot/smoke-tests#212

Merged

kbukum1 marked this pull request as ready for review June 26, 2024 17:27

kbukum1 requested a review from a team as a code owner June 26, 2024 17:27

kbukum1 force-pushed the kbukum1/fix_for_pip_pyproject_bump_versions_strategy branch 2 times, most recently from 24ed6c0 to f304dc8 Compare June 26, 2024 22:04

abdulapopoola approved these changes Jun 27, 2024

View reviewed changes

abdulapopoola requested review from abdulapopoola and jeffwidman June 27, 2024 00:56

honeyankit reviewed Jun 27, 2024

View reviewed changes

python/lib/dependabot/python/update_checker/requirements_updater.rb Show resolved Hide resolved

kbukum1 requested a review from honeyankit June 27, 2024 22:11

honeyankit approved these changes Jun 27, 2024

View reviewed changes

honeyankit self-requested a review June 27, 2024 22:18

kbukum1 force-pushed the kbukum1/fix_for_pip_pyproject_bump_versions_strategy branch from 91394ad to f304dc8 Compare June 27, 2024 22:45

kbukum1 added 2 commits June 27, 2024 16:21

Added fix for lower pyproject low boundry issue for bump version (inc…

a740c39

…rease strategy).

added specs for unfixable requirements

bb91a7e

kbukum1 force-pushed the kbukum1/fix_for_pip_pyproject_bump_versions_strategy branch from ead807d to bb91a7e Compare June 27, 2024 23:21

lower version check is fixed.

3936a85

honeyankit approved these changes Jun 27, 2024

View reviewed changes

kbukum1 merged commit b6c464a into main Jun 27, 2024
76 of 78 checks passed

kbukum1 deleted the kbukum1/fix_for_pip_pyproject_bump_versions_strategy branch June 27, 2024 23:49

deivid-rodriguez reviewed Jun 28, 2024

View reviewed changes

sanderr mentioned this pull request Jul 3, 2024

The increase strategy should move ranges forward, not widen them #6631

Closed

1 task

deivid-rodriguez mentioned this pull request Jul 8, 2024

[python] - Fix for updating lower constraint only for increase strategy #10154

Closed

5 tasks

jonjanego mentioned this pull request Jul 10, 2024

Audit the version update behavior across ecosystems #10187

Open

5 tasks

This was referenced Jul 11, 2024

Update smoke tests for "Increase" versioning strategy adjustment in python dependencies dependabot/smoke-tests#214

Closed

Revert PR #10060: Fix Versioning Strategy for Python Dependencies #10194

Merged

kbukum1 added a commit that referenced this pull request Jul 11, 2024

Revert PR #10060: Fix Versioning Strategy for Python Dependencies (#1…

a1db7d0

…0194) * Revert "Added fix for lower pyproject low boundry issue for bump version (increase strategy)."

kbukum1 mentioned this pull request Jul 11, 2024

Changing the lower bound on a requirement without being asked to do so is rude #10130

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update lower boundary in Python pip pyproject when bumping versions #6631 #10060

Update lower boundary in Python pip pyproject when bumping versions #6631 #10060

kbukum1 commented Jun 22, 2024 •

edited

Loading

kbukum1 commented Jun 26, 2024 •

edited

Loading

abdulapopoola commented Jun 27, 2024

honeyankit commented Jun 27, 2024 •

edited

Loading

kbukum1 commented Jun 27, 2024

kbukum1 commented Jun 27, 2024

deivid-rodriguez left a comment

deivid-rodriguez Jun 28, 2024

deivid-rodriguez Jun 28, 2024

deivid-rodriguez Jun 28, 2024

deivid-rodriguez Jun 28, 2024

deivid-rodriguez Jun 28, 2024

kbukum1 Jun 28, 2024

deivid-rodriguez Jul 2, 2024

sanderr Jul 3, 2024

sanderr Jul 3, 2024 •

edited

Loading

deivid-rodriguez Jul 3, 2024

sanderr Jul 3, 2024

kbukum1 Jul 5, 2024

sanderr Jul 8, 2024

deivid-rodriguez Jul 8, 2024

deivid-rodriguez Jun 28, 2024

deivid-rodriguez Jun 28, 2024

deivid-rodriguez Jun 28, 2024

deivid-rodriguez Jun 28, 2024

deivid-rodriguez Jun 28, 2024

deivid-rodriguez commented Jun 28, 2024 •

edited

Loading

jeffwidman commented Jun 28, 2024

kbukum1 commented Jul 2, 2024

kbukum1 commented Jul 11, 2024

Update lower boundary in Python pip pyproject when bumping versions #6631 #10060

Update lower boundary in Python pip pyproject when bumping versions #6631 #10060

Conversation

kbukum1 commented Jun 22, 2024 • edited Loading

What are you trying to accomplish?

Anything you want to highlight for special attention from reviewers?

How will you know you've accomplished your goal?

Checklist

kbukum1 commented Jun 26, 2024 • edited Loading

abdulapopoola commented Jun 27, 2024

honeyankit commented Jun 27, 2024 • edited Loading

kbukum1 commented Jun 27, 2024

kbukum1 commented Jun 27, 2024

deivid-rodriguez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanderr Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deivid-rodriguez commented Jun 28, 2024 • edited Loading

jeffwidman commented Jun 28, 2024

kbukum1 commented Jul 2, 2024

kbukum1 commented Jul 11, 2024

kbukum1 commented Jun 22, 2024 •

edited

Loading

kbukum1 commented Jun 26, 2024 •

edited

Loading

honeyankit commented Jun 27, 2024 •

edited

Loading

sanderr Jul 3, 2024 •

edited

Loading

deivid-rodriguez commented Jun 28, 2024 •

edited

Loading