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

Extend 2.21 deprecations: remote_oauth_bearer_token_path, crossversion=partial #20616

Merged
merged 4 commits into from
Feb 27, 2024

Conversation

huonw
Copy link
Contributor

@huonw huonw commented Feb 26, 2024

There were three deprecations originally scheduled for 2.21 (#20609), two of will have only been deprecated for one release, and aren't a significant burden to support, so we can ease users life for a little longer:

description deprecation started in what could be removed
the [GLOBAL].remote_oauth_bearer_token_path option 2.20.0.dev1 (#20116) configuration for the option, plus some a few small if statements, all in global_options.py
passing crossversion="partial" for scala artifacts 2.20.0 (#20264) NB. due to an easy-to-make misuse of start_version I believe people will only be getting the deprecation warnings in 2.20.0 stable, not any of the release candidates an extra enum variant, and a test

The third (python_awslambda) is removed in #20619.

@huonw huonw mentioned this pull request Feb 26, 2024
@huonw huonw added the category:internal CI, fixes for not-yet-released features, etc. label Feb 26, 2024
@kaos
Copy link
Member

kaos commented Feb 26, 2024

I'm generally in favour of keeping the deprecation period longer to give ample time for users to migrate as appropriate. I'm less in favour of changing deprecation windows (unless there is a strong reason to). We should be vigilant when setting them, rather than pushing them when we reach the current end version, as that erodes the credibility when things will go away. I as user risk getting into the habit of, "nah, they say this will go away at X, but it'll likely be pushed out so I don't need to do anything about it yet.."

@huonw
Copy link
Contributor Author

huonw commented Feb 26, 2024

Thanks for the input. I agree we should be careful about choosing deprecation periods, but I disagree (fairly strongly!) that we should feel bad about extending them.

For this specific case, I will:

  • switch to just removing python_awslamdba (one extension is enough, and its now been deprecated for 3 versions)
  • keep the extension of the other two: I think "only being deprecated for one version" is a strong enough reason to extend the deprecation, and we haven't released 2.20 so we can cherrypick the extension, and most users won't even notice.

That said, for the general case: I think Pants is too happy to break backwards compatibility and that makes upgrading hard, and upgrading being hard means people will stay on old versions ~forever, which is bad for them and bad for the project as a whole.

As examples of the breakage we've forced on users, in 2.16 and later, we've made major changes to the Python backend like: changing tools lockfiles work (2.16), Lambda/GCF artifacts (2.17 & 2.18), changing the dep inference parser (2.18), deprecating pex_binary(platforms=...) (2.19). For all except the last, I think we've only given users a single release to find bugs and/or adapt to the changes (without warnings), and this has lead to more questions and frustration on slack and in the issue tracker.

So, my impression is we're still far from people thinking "I can ignore these warnings because sometimes the deprecations are extended" being the major problem with how we approach deprecations!

Have you had a different impression from users?

@huonw huonw changed the title Extend 2.21 deprecations: python_awslambda target, remote_oauth_bearer_token_path, crossversion=partial Extend 2.21 deprecations: remote_oauth_bearer_token_path, crossversion=partial Feb 26, 2024
@huonw huonw marked this pull request as ready for review February 26, 2024 22:45
@huonw huonw requested review from kaos and alonsodomin February 26, 2024 22:45
@huonw
Copy link
Contributor Author

huonw commented Feb 26, 2024

(Done the split: #20619)

@huonw huonw added this to the 2.20.x milestone Feb 26, 2024
Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

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

I think Pants is too happy to break backwards compatibility

I 100% agree with this. And no, I've not yet really had any experience with my comment about users getting lax about deprecations. My main point though, I think we both agree, is that perhaps for the most part having a single version deprecation window is too short, and we should adapt our deprecation policy to match. Perhaps change the wording to something that encourages longer periods, and guarantees that it will never be shorter than one stable version, and only for exceptional cases at that. It now reads:

Deprecations must live at least one minor release, meaning that if something is deprecated in 2.1.x, for example, it cannot be removed until 2.2.x.

Having push back to push the deprecation window will, I believe, shift focus towards being more conservative when introducing the deprecation. As it is, I feel like it is tossed with not enough consideration perhaps as "we can always change this later". Of course we can, but I think we should reserve those cases to when things come up that we didn't foresee and we need more time for us or the users or both.

For this particular case I agree with your assessment that we can adjust the deprecation window with little to no impact as it has not yet reached a stable version.

Looking at the diff now, I didn't realize before but, I see now that the end version was a stable version, not .dev0 as it should be, further adding to the use of a lint check that ensures our deprecations target dev releases so we can catch them before going stable. Nice catch :)

@huonw huonw merged commit ab95266 into main Feb 27, 2024
24 checks passed
@huonw huonw deleted the huonw/extend-deprecations branch February 27, 2024 10:24
WorkerPants pushed a commit that referenced this pull request Feb 27, 2024
…n=partial (#20616)

There were three deprecations originally scheduled for 2.21 (#20609),
two of will have only been deprecated for one release, and aren't a
significant burden to support, so we can ease users life for a little
longer:

| description                                          | deprecation started in | what could be removed                                                                           |
|------------------------------------------------------|------------------------|-------------------------------------------------------------------------------------------------|
| the `[GLOBAL].remote_oauth_bearer_token_path` option | 2.20.0.dev1 (#20116)   | configuration for the option, plus some a few small `if` statements, all in `global_options.py` |
| passing `crossversion="partial"` for scala artifacts | 2.20.0 (#20264) ^      | an extra enum variant, and a test                                                               |

^ NB. due to an easy-to-make misuse of `start_version` I believe people will only be getting the deprecation warnings in 2.20.0 stable, not any of the release candidates.

The third (`python_awslambda`) is removed in #20619.
@WorkerPants
Copy link
Member

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.20.x

Successfully opened #20623.


Thanks again for your contributions!

🤖 Beep Boop here's my run link

@huonw
Copy link
Contributor Author

huonw commented Feb 27, 2024

My main point though, I think we both agree, is that perhaps for the most part having a single version deprecation window is too short, and we should adapt our deprecation policy to match. Perhaps change the wording to something that encourages longer periods, and guarantees that it will never be shorter than one stable version, and only for exceptional cases at that

Yeah, you're right we both agree 👍

Having push back to push the deprecation window will, I believe, shift focus towards being more conservative when introducing the deprecation. As it is, I feel like it is tossed with not enough consideration perhaps as "we can always change this later". Of course we can, but I think we should reserve those cases to when things come up that we didn't foresee and we need more time for us or the users or both.

I see where you're coming from. 👍

huonw added a commit that referenced this pull request Feb 27, 2024
…n=partial (Cherry-pick of #20616) (#20623)

There were three deprecations originally scheduled for 2.21 (#20609),
two of will have only been deprecated for one release, and aren't a
significant burden to support, so we can ease users life for a little
longer:

| description                                          | deprecation started in | what could be removed                                                                           |
|------------------------------------------------------|------------------------|-------------------------------------------------------------------------------------------------|
| the `[GLOBAL].remote_oauth_bearer_token_path` option | 2.20.0.dev1 (#20116)   | configuration for the option, plus some a few small `if` statements, all in `global_options.py` |
| passing `crossversion="partial"` for scala artifacts | 2.20.0 (#20264) ^      | an extra enum variant, and a test                                                               |

^ NB. due to an easy-to-make misuse of `start_version` I believe people will only be getting the deprecation warnings in 2.20.0 stable, not any of the release candidates

The third (`python_awslambda`) is removed in #20619.

Co-authored-by: Huon Wilson <huon@exoflare.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants