Update address parameters on overrides #20934

isra17 · 2024-05-19T00:40:33Z

This fix a bug where default parametrize resolve could not get overriden.

The fix ensure that an address parameters are updated on any override, not just parametrized one.

Fixes #20933

benjyw · 2024-05-20T00:39:47Z

Thanks for the fix @isra17 ! To get CI passing, add an entry in docs/notes/2.21.x.md (since this is a bugfix we'll cherrypick it to 2.21.0 before releasing that), and also fix whatever the lint shard is complaining about (pants --changed-since=main fmt fix check lint should do the trick).

huonw · 2024-05-20T06:32:56Z

Thanks for the fix!

It looks like this reproduces in at least 2.17.0 (thanks for the reproducer in #20933), which suggests to me:

let's cherry pick back to 2.19
we shouldn't try to get this into 2.21.0 stable:
- as release manager, I'm feeling like we're pretty close to being good to release 2.21.0, with minimal bugs reported about it, specifically (and 2.21 release preparation #20846 suggest 6 weeks since 2.20.0 would be May 28, next week)
- any change risks delaying the release of all the new features (e.g. if that change has a minor mistake), and so preferably we only do necessary/urgent ones
- this doesn't feel like a urgent bug fix, because it's not a regression since 2.20: if we release 2.21.0 with it not-fixed, no existing code is going to be newly affected (it's already affected in 2.20 and earlier!).

Thus, I'd suggest we put the notes in docs/notes/2.22.x.md since 2.22.0 will be the first .0 release that contains it.

kaos

Thanks for the find, repro! and fix 🙌🏽

I think we'd like to take it the full mile however, and remove any "unparametrized" parameters from the address as well.

src/python/pants/engine/internals/parametrize_test.py

isra17 · 2024-05-20T18:08:22Z

Ok, so this PR grew a bit in scope trying to attempt #20934 (comment) .

In short, I've added support to:

Unparametrize address when expanding them with single values: Address("a@f=x").expand({"f": "y"}) => Address(a).
Manage single value Parametrize the same as a single value (Handle f="x" the same as f=Parametrize("x")).

I also went a bit farther and unparametrize from group as well, so that if a parameters group has resolve="other-resolve", then it's going to remove the resolve parameter from the address. This might fix other similar bug than the original one.

Let me know what you think and if you would rather have me revert to the original scope.

kaos · 2024-05-21T06:30:31Z

Ok, so this PR grew a bit in scope trying to attempt #20934 (comment) .

In short, I've added support to:

Unparametrize address when expanding them with single values: Address("a@f=x").expand({"f": "y"}) => Address(a).

Manage single value Parametrize the same as a single value (Handle f="x" the same as f=Parametrize("x")).

I also went a bit farther and unparametrize from group as well, so that if a parameters group has resolve="other-resolve", then it's going to remove the resolve parameter from the address. This might fix other similar bug than the original one.

Let me know what you think and if you would rather have me revert to the original scope.

Love this!

I think we may want to go back to not unparametrize single value parametrizations, but lets hold on that until we get more opinions.

kaos

yea, looking at this code I feel it's kind of hairy.

Perhaps split this to fix the original issue in this PR, and have a new PR addressing the more general case?
wdyt?

docs/notes/2.22.x.md

src/python/pants/engine/internals/parametrize.py

isra17 · 2024-05-22T01:16:57Z

Ok, third round!

I removed the new logic that normalize single value parametrize into non-parametrize (I can open another PR with these change if there's any interest for this behavior).
I reimplemented the change with the suggestion of tracking a parameters dict and creating a new address once.

I think CI should get green this time 🤔

kaos

This looks great, thanks!

Just some minor nits and tweaks left, I think :)

docs/notes/2.22.x.md

src/python/pants/engine/internals/parametrize.py

src/rust/engine/src/externs/address.rs

This fix a bug where default parametrize resolve with overrides would have inconsistent parameters and field values.

isra17 · 2024-05-26T19:55:54Z

Comment addressed and CI is green!

src/python/pants/engine/internals/parametrize.py

kaos

Great.

(n.b. would prefer regular pushes over force push when possible, makes it easier to review what's changed since last review. we do squash merges so it doesn't mess up the history either way.)

WorkerPants · 2024-05-28T09:04:14Z

I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants.

❌ 2.19.x

I was unable to cherry-pick this PR to 2.19.x, likely due to merge-conflicts.

Steps to Cherry-Pick locally

To resolve:

(Ensure your git working directory is clean)

Run the following script to reproduce the merge-conflicts:

git fetch https://github.com/pantsbuild/pants main \
  && git fetch https://github.com/pantsbuild/pants 2.19.x \
  && git checkout -b cherry-pick-20934-to-2.19.x FETCH_HEAD \
  && git cherry-pick 408d29182de5ea9f170e253f303412ed56407181

Fix the merge conflicts and commit the changes
Run build-support/cherry_pick/make_pr.sh "20934" "2.19.x"

Please note that I cannot re-run CI if a job fails. Please work with your PR approver(s) to re-run CI if necessary.

❌ 2.20.x

I was unable to cherry-pick this PR to 2.20.x, likely due to merge-conflicts.

Steps to Cherry-Pick locally

To resolve:

(Ensure your git working directory is clean)

Run the following script to reproduce the merge-conflicts:

git fetch https://github.com/pantsbuild/pants main \
  && git fetch https://github.com/pantsbuild/pants 2.20.x \
  && git checkout -b cherry-pick-20934-to-2.20.x FETCH_HEAD \
  && git cherry-pick 408d29182de5ea9f170e253f303412ed56407181

Fix the merge conflicts and commit the changes
Run build-support/cherry_pick/make_pr.sh "20934" "2.20.x"

Please note that I cannot re-run CI if a job fails. Please work with your PR approver(s) to re-run CI if necessary.

❌ 2.21.x

I was unable to cherry-pick this PR to 2.21.x, likely due to merge-conflicts.

Steps to Cherry-Pick locally

To resolve:

(Ensure your git working directory is clean)

Run the following script to reproduce the merge-conflicts:

git fetch https://github.com/pantsbuild/pants main \
  && git fetch https://github.com/pantsbuild/pants 2.21.x \
  && git checkout -b cherry-pick-20934-to-2.21.x FETCH_HEAD \
  && git cherry-pick 408d29182de5ea9f170e253f303412ed56407181

Fix the merge conflicts and commit the changes
Run build-support/cherry_pick/make_pr.sh "20934" "2.21.x"

Please note that I cannot re-run CI if a job fails. Please work with your PR approver(s) to re-run CI if necessary.

When you're done manually cherry-picking, please remove the needs-cherrypick label on this PR.

Thanks again for your contributions!

🤖 Beep Boop here's my run link

isra17 · 2024-05-28T13:03:58Z

Great.

(n.b. would prefer regular pushes over force push when possible, makes it easier to review what's changed since last review. we do squash merges so it doesn't mess up the history either way.)

Thank you for your patience!

In the future I will keep commits stack if that make your job easier ;)

kaos · 2024-05-29T07:43:33Z

@huonw I don't have the bandwidth currently to look into the failed cherry picks. Could be that the merge conflicts are due to changes in the parametrize.py that hasn't been picked.

Not sure how urgent it is to actually get this fix picked?

@isra17 are you tied down to a particular Pants version, and would prefer this fix on that release branch, or are you able to upgrade to an upcoming 2.22 dev/rc?

huonw · 2024-05-29T10:30:55Z

If there's changes that haven't been picked, that's a possibility.

Another likely candidate is the release notes addition: the 2.22 file won't exist in older versions. This is a "known issue" unfortunately, e.g. #20888 (comment)

kaos · 2024-05-29T14:50:39Z

Ah, that could be it as well then. I thought I had some relevant changes, but I think those've been picked.

isra17 · 2024-05-29T15:41:15Z

No worry on our side, we did went with a workaround so it's not a blocker (And we also don't mind to upgrade to dev/rc)

huonw requested a review from kaos May 19, 2024 22:33

benjyw added category:bugfix Bug fixes for released features needs-cherrypick labels May 20, 2024

benjyw added this to the 2.21.x milestone May 20, 2024

huonw modified the milestones: 2.21.x, 2.19.x May 20, 2024

kaos reviewed May 20, 2024

View reviewed changes

src/python/pants/engine/internals/parametrize_test.py Show resolved Hide resolved

src/python/pants/engine/internals/parametrize_test.py Outdated Show resolved Hide resolved

kaos reviewed May 20, 2024

View reviewed changes

src/python/pants/engine/internals/parametrize_test.py Outdated Show resolved Hide resolved

kaos reviewed May 21, 2024

View reviewed changes

docs/notes/2.22.x.md Outdated Show resolved Hide resolved

src/python/pants/engine/internals/parametrize.py Outdated Show resolved Hide resolved

isra17 force-pushed the isra/fix-default-parametrize branch from 73e7816 to da4f42c Compare May 22, 2024 01:15

isra17 force-pushed the isra/fix-default-parametrize branch from da4f42c to ea39d3e Compare May 22, 2024 01:19

kaos approved these changes May 22, 2024

View reviewed changes

Remove overriden parameters from address.

3cf30c3

This fix a bug where default parametrize resolve with overrides would have inconsistent parameters and field values.

isra17 force-pushed the isra/fix-default-parametrize branch from ad16011 to d95d095 Compare May 26, 2024 19:01

kaos approved these changes May 27, 2024

View reviewed changes

src/python/pants/engine/internals/parametrize.py Show resolved Hide resolved

isra17 force-pushed the isra/fix-default-parametrize branch from d95d095 to 3cf30c3 Compare May 27, 2024 14:18

kaos approved these changes May 28, 2024

View reviewed changes

kaos merged commit 408d291 into pantsbuild:main May 28, 2024
25 checks passed

WorkerPants added the auto-cherry-picking-failed Auto Cherry-Picking Failed label May 28, 2024

isra17 deleted the isra/fix-default-parametrize branch May 28, 2024 13:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update address parameters on overrides #20934

Update address parameters on overrides #20934

isra17 commented May 19, 2024

benjyw commented May 20, 2024

huonw commented May 20, 2024

kaos left a comment

isra17 commented May 20, 2024

kaos commented May 21, 2024

kaos left a comment

isra17 commented May 22, 2024

kaos left a comment

isra17 commented May 26, 2024

kaos left a comment

WorkerPants commented May 28, 2024

isra17 commented May 28, 2024

kaos commented May 29, 2024

huonw commented May 29, 2024

kaos commented May 29, 2024

isra17 commented May 29, 2024

Update address parameters on overrides #20934

Update address parameters on overrides #20934

Conversation

isra17 commented May 19, 2024

benjyw commented May 20, 2024

huonw commented May 20, 2024

kaos left a comment

Choose a reason for hiding this comment

isra17 commented May 20, 2024

kaos commented May 21, 2024

kaos left a comment

Choose a reason for hiding this comment

isra17 commented May 22, 2024

kaos left a comment

Choose a reason for hiding this comment

isra17 commented May 26, 2024

kaos left a comment

Choose a reason for hiding this comment

WorkerPants commented May 28, 2024

❌ 2.19.x

❌ 2.20.x

❌ 2.21.x

isra17 commented May 28, 2024

kaos commented May 29, 2024

huonw commented May 29, 2024

kaos commented May 29, 2024

isra17 commented May 29, 2024