Deprecate the platforms field on pex_binary. by benjyw · Pull Request #20012 · pantsbuild/pants

benjyw · 2023-10-11T00:40:52Z

The abbreviated platforms were a twitter-internal hack that
has unfortunately spilled over into our public API. They
don't work in the general case, and should be replaced
by use of complete_platforms.

jsirois

CI looks bloody but it's hard to see what you did here that might be related.

jsirois · 2023-10-11T02:52:49Z


 class PexPlatformsField(StringSequenceField):
    alias = "platforms"
+    removal_version = "2.20.0.dev0",


Aha, it's the commas. You made 1-tuples on this line and the next.

yep, my brain isn't working so well right now

But also it turns out something may also be broken in the deprecation logic. As well as my brain.

Yep: yeeeesh

Will fix.

Previously it was in a special-cased python_requirement target for no apparent reason. This allows it to be used for debugging pants itself's non-test code (previously it was only useful for debugging pants tests).

huonw · 2023-10-11T20:13:34Z


 class PexPlatformsField(StringSequenceField):
    alias = "platforms"
+    removal_version = "2.20.0.dev0"


I imagine this may be quite impactful across the pants ecosystem, and I believe the main motivation for this is that it is finicky to work with and easy to give the wrong result, rather than significant Pants-internal code simplification?

As such, maybe we could have a longer deprecation period to ease user churn?

Suggested change

removal_version = "2.20.0.dev0"

removal_version = "2.23.0.dev0"

(Made up, but approximately 6 months, with 6 week releases?)

True, this doesn't lead to simplification in Pants, but it does lead to simplification of our lives. Since platforms is broken, it is a bit of a landmine in the user's codebase, and when it breaks they first turn to Slack, which costs us time and support resources. So we do want to shovel them off it ASAP.

I could go for 2.22?

Yeah, that makes sense. Happy with 2.22! (Given the warning isn't easily disablable, I would imagine that most people switch away in 2.19...)

huonw · 2023-10-11T21:48:10Z


 class PexPlatformsField(StringSequenceField):
    alias = "platforms"
+    removal_version = "2.20.0.dev0"


Yeah, that makes sense. Happy with 2.22! (Given the warning isn't easily disablable, I would imagine that most people switch away in 2.19...)

huonw · 2023-10-11T21:52:05Z

 class PexPlatformsField(StringSequenceField):
    alias = "platforms"
+    removal_version = "2.20.0.dev0"
+    removal_hint = "Use complete_platforms instead."


I think it might be nice to explain this a bit more, in two ways:

link directly to the new multi-platform PEX docs (so a user trying to migrate is not having to first look at the complete_platforms doc string and then find the bigger-explanation docs from there)

either: expand those docs to discuss "why we're deprecating platforms" (temporarily, at least, e.g. something like https://www.pantsbuild.org/v2.18/docs/awslambda-python#migrating-from-pants-216-and-earlier) or have a sentence or two like that inline here.

Putting myself in the user's shoes, I would think that this migration will feel quite churn-ful for users that have have code that currently works ("this is working fine for me, why do I have to change"), and having easier links and understanding more background will, I hope, alleviate some of that frustration.

(Sorry for dribbling out these comments across multiple reviews!)

I've shied away from this sort of thing and Pants as a whole, but completely agreed. Deprecations as they currently are used are a crutch in my mind that Pants should grow out of. They allow less than good decisions to continue to be made in general and mask the pain that should be felt by devs by shunting it to users.

I agree that ~every deprecation represents a failure on our part to get an API decision right. But the design space is huge, and as we feel our way through it we will make mistakes.

To use this case as an example: we all agree that users shouldn't use platforms. That was a design mistake in Pex (or at least in post-Twitter Pex), that Pants carried over. There are two ways we can achieve this end:

A) Let user keep on trying to use platforms, occasionally failing in non-obvious ways, and coming to Slack for help, which takes up a lot of our time in providing the same advice over and over.
B) A deprecation, which is flashing warning sign around the mistake that lets new users know proactively not to use it.

We can debate how far out the deprecation should go. Certainly in this case it's easy to keep it for as long as we're on Pex 2 (my understanding is that Pex 3 won't support platforms?). That's one extreme. I lean towards taking away the bad thing within a reasonable time frame, with enough warning, reasonable people can disagree on that.

I do see your point about mitigating the frustration with better docs. I will look into that (after I fix another breakage caused by #19143)

I'm not sure whether or not Pex 3 will support platforms. But I think you know I'm of the opinion taking away a thing that works for a user (@huonw's point), is beyond maddening for that user. Doc, make non-default, etc - that's progress. Breaking an existing user is not. I'd not yank until Pants 3 - basically actually respect the side of semver that favors users looking for stability and not favoring developers of Pants.

huonw

Thanks.

Happy with this as it is, and we can always extend the removal version in future if we want to be more generous to users.

benjyw · 2023-10-23T15:39:19Z

True, we have until 2.22.x to debate it, and see how users respond.

At least this gives them a loud message about not using platforms, and I would be comfortable continuing that for longer if necessary.

…solve_local_platforms` (#20823) This field was deprecated in #20012, released in 2.19. We've now branched for 2.21 and thus `main` is prepping for 2.22. So, the deprecation has expired! In that PR, there was discussion about working out whether this should remain for longer. This has been deprecated for several releases now, and it's being removed early in the 2.22 release cycle: if someone comments, we can reinstate/reconsider. --- Also, as part of this, I noticed the `resolve_local_platforms` field and option now seem to be meaningless: the underlying `pex --resolve-local-platforms` field is documented as `When --platforms are specified, ...`. Thus, this deprecates that field and the global option for 2.24.

Deprecate the platforms field on pex_binary.

347af8a

benjyw added the category:user api change label Oct 11, 2023

benjyw requested review from huonw and jsirois October 11, 2023 00:40

jsirois approved these changes Oct 11, 2023

View reviewed changes

jsirois reviewed Oct 11, 2023

View reviewed changes

benjyw added 3 commits October 10, 2023 21:53

Commas, yeesh

0c4ca27

Move the pydevd-pycharm requirement into requirements.txt.

3d0c7e8

Previously it was in a special-cased python_requirement target for no apparent reason. This allows it to be used for debugging pants itself's non-test code (previously it was only useful for debugging pants tests).

Merge branch 'main' into deprecate_platforms

a2ee6c3

huonw approved these changes Oct 11, 2023

View reviewed changes

huonw reviewed Oct 11, 2023

View reviewed changes

benjyw added 7 commits October 11, 2023 15:12

Merge branch 'benjyw_pydevd' into deprecate_platforms

5aa6b22

Merge branch 'main' into deprecate_platforms

08efa2e

Merge branch 'main' into deprecate_platforms

c67e663

Merge branch 'main' into deprecate_platforms

77753a6

Merge branch 'main' into deprecate_platforms

6571204

Code review feedback

37b9314

lint

7ae55d4

huonw approved these changes Oct 23, 2023

View reviewed changes

benjyw merged commit 255db66 into main Oct 23, 2023

benjyw deleted the deprecate_platforms branch October 23, 2023 15:39

cognifloyd mentioned this pull request Feb 9, 2024

Add [python].resolves_to_target_os to tell Pex whether to generate lockfile for macOS, Linux, or both #17276

Open

huonw mentioned this pull request Apr 20, 2024

Remove deprecated pex_binary(platforms=...) for 2.22, deprecate resolve_local_platforms #20823

Merged

	removal_version = "2.20.0.dev0"
	removal_version = "2.23.0.dev0"

Uh oh!

Conversation

benjyw commented Oct 11, 2023

Uh oh!

jsirois left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsirois Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsirois Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huonw left a comment

Choose a reason for hiding this comment

Uh oh!

benjyw commented Oct 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jsirois Oct 11, 2023 •

edited

Loading

jsirois Oct 19, 2023 •

edited

Loading