https://pantsbuild.org/ logo
#development
Title
# development
h

hundreds-father-404

06/15/2021, 8:33 PM
In v1,
interpreter_constraints
on a
pex_binary
target would override the transitive closure. In v2, we "fixed" it to instead act like all other interpreter constraints, to be mixed in. The practical impact is described in https://www.pantsbuild.org/docs/python-interpreter-compatibility#using-multiple-python-versions-in-the-same-project
This means that every dependency of a target must also be compatible with its interpreter constraints.
Thoughts if that was a mistake or not?
e

enough-analyst-54434

06/15/2021, 8:34 PM
Before answering that, its good to firm up epistimlogies. Why was the 1st decision considered rock solid at the time?
h

hundreds-father-404

06/15/2021, 8:34 PM
Context is that I want to add
interpreter_constraints
to
python_distribution
to likely fix an issue @big-xylophone-43403 reported, and because I think it's sensible for
python_distribution
to have the field similar to how
pex_binary
has it So I'm revisiting if we should override vs. mix-in
Why was the 1st decision considered rock solid at the time?
Which decision: v1 to override, or the decision to change v2 to add in?
e

enough-analyst-54434

06/15/2021, 8:35 PM
The decision you made
Its not a trick question. I think there's really no opinion involved. Your decision was correct. The IC has to be the intersection.
h

hundreds-father-404

06/15/2021, 8:38 PM
(Trying to find the issue where I made the change to behavior)
e

enough-analyst-54434

06/15/2021, 8:38 PM
If its anything else and the code still works one or more of the ICs invloved is a lie.
👍 1
1
Never revisit decisions that are based on pure correct logic. These thigs should not flap in the wind.
h

hundreds-father-404

06/15/2021, 8:41 PM
This is interesting from V1 land, we went out of our way to only use `pex_binary`: https://github.com/pantsbuild/pants/issues/7775
But I agree that I think the current behavior is preferable. Maybe if a user has a need for something like #7775 again, we add a new field like
interpreter_constraints_override
(exclusive w/
interpreter_constraints
). In the meantime, stick with current sensible behavior. Sg?
e

enough-analyst-54434

06/15/2021, 8:44 PM
How do you read that? Stus OP suggests verifying intersection in point 2. But point 1 is only about cheaply side-stepping the OR issue.
And you can only do Stus step 1 because of checking step 2
IIUC you implemented a better step 1.
h

hundreds-father-404

06/15/2021, 8:46 PM
Did I? We build the Pex with the whole closure and that constraint gets baked into the PEX-INFO last I checked. I agree #2 is solved tho
e

enough-analyst-54434

06/15/2021, 8:48 PM
OK, So The issue Stu file about was we used all constraints, but in a list - so they got ORed - which was wrong. His fix suggestion was go the step 2 check and just pick one constraint for to write down after that - the easiest one to pick being the pex_binary one. I thought you improved that by implementing merging logic with Benjy's help with some demorgans law stuff?
h

hundreds-father-404

06/15/2021, 8:53 PM
Oh! Okay I see what you're getting at. Before, that BUILD file would result in:
['CPython>=2.7,<4', 'CPython>=3.6']
Now it correctly results in:
['CPython>=2.7,>=3.6,<4']
e

enough-analyst-54434

06/15/2021, 8:54 PM
Yeah, like that. So you improved things with merging.
👍 1
So, to my original point, all this logic was correct. To step back from it is to say, come have a seat at the table mr bug. Clearly no.
1
✔️ 1
h

hundreds-father-404

06/15/2021, 8:55 PM
Cool, thank you for walking through that. I'm convinced
e

enough-analyst-54434

06/15/2021, 8:56 PM
OK. You need to watch out. You have old man brain like me and forget what you thought up yesterday already!
h

hundreds-father-404

06/15/2021, 8:57 PM
Yeah, too many things like this floating around in my head Solid PR descriptions and issues are a helpful remedy
e

enough-analyst-54434

06/15/2021, 9:16 PM
So presumably you'll be mapping IC to Requires-Python metadata for the distribution? Is that what the bug is about?
h

hundreds-father-404

06/15/2021, 9:20 PM
We still don't autogenerate Requires-Python, which we maybe should be. Hm. A complication there is it expects a single requirement, whereas the constraints could be a list A workaround is that users can manually set
provides=setup_py()
-- I'm waiting for user to confirm in https://pantsbuild.slack.com/archives/C046T6T9U/p1621624847091700, but I'm pretty sure the bug is that they have a
python_distribution
with no dependencies on
python_library
targets. Meaning transitive closure has no constraints. So Py2 is getting picked up for them, which they can't use Adding
interpreter_constraints
to
python_distribution
fixes this
e

enough-analyst-54434

06/15/2021, 9:30 PM
Hrm. A pure resource distribution? That's certainly valid but odd.
h

hundreds-father-404

06/15/2021, 9:30 PM
Agreed it's odd. But because it's legal, we should fix imo
e

enough-analyst-54434

06/15/2021, 9:31 PM
I mean, a pure resource distribution has no IC by definition though.
It's just files.
Adding an IC sounds like a hack.
Isn't Benjy's suggestion exactly correct? It looks like it got no response / was maybe missed?: https://pantsbuild.slack.com/archives/C046T6T9U/p1621629309094000?thread_ts=1621624847.091700&amp;cid=C046T6T9U
h

hundreds-father-404

06/15/2021, 9:38 PM
That works fine until you set
setup_py_commands
because we need to know which interpreter to run
setuptools
. You removed
[setuptools].interprerter_constraints
in 2.4 for correctness reasons
it seems to me that the global constraints should still apply in this case?
That is the impact of my PR, we will use global constraints in the default case, whereas we currently would use no constraints at all when running setuptools. I'll put this up rn
e

enough-analyst-54434

06/15/2021, 9:39 PM
Why does adding the setup_py_commands field change the IC answer?
h

hundreds-father-404

06/15/2021, 9:41 PM
we don't run setuptools if
setup_py_commands
isn't set, so the ICs are completely ignored. ICs are only used with
setup_py.py
to determine what to run
setuptools
with + if we have Py2 for namespace packages handling
e

enough-analyst-54434

06/15/2021, 9:43 PM
I'm totally confused. There is an algorithm to determine IC using target deps. If none, then the default IC. Now, mix in setup_py_commands - the answer should not change.
h

hundreds-father-404

06/15/2021, 9:46 PM
There is an algorithm to determine IC using target deps. If none, then the default IC.
Ah, that may be the source of confusion. The algorithm only uses
[python-setup].interpreter_constraints
as a default for targets who have an
interpreter_constraints
field defined. Otherwise it skips the target. So, if you have only a single
python_distribution
target w/ no deps, you get empty constraints back
e

enough-analyst-54434

06/15/2021, 9:48 PM
So change the algorithm!
In short, Benjy's answer is correct and you're getting tripped up by circumstantial facts on the ground.
Those facts are mutable though.
h

hundreds-father-404

06/15/2021, 10:07 PM
Hm, my response to that was realizing it'd be great to do this rather than how
release.sh
sets `PYTHON_SETUP_INTERPRETER_CONSTRAINTS`:
Copy code
python_distribution(
    name="pants-packaged-py37", interpreter_constraints=["==3.7.*"], **_pants_packaged_kwargs
)
python_distribution(
    name="pants-packaged-py38", interpreter_constraints=["==3.8.*"], **_pants_packaged_kwargs
)
python_distribution(
    name="pants-packaged-py39", interpreter_constraints=["==3.9.*"], **_pants_packaged_kwargs
)
But that fails due to multiple owners of the same first party code 🤔 -- Even with that, I do think there's value in letting you set constraints on an individual
python_distribution
. Examples: * Your library code works with 3.6+. But you must run setuptools with specifically 3.8, e.g. for the correct native platform to be used. You shouldn't need to constrain your
python_library
to do that * You have no first party code in your dist. Your repo's default is Py2 or Py3.5+. But you're using a newer version of setuptools so you can't run it with Py2 These types of situations are why we added back
interpreter_constraints
to
pex_binary
and
python_awslambda
. @jolly-midnight-72759 and @helpful-lunch-92084 shared that they were setting that field a lot and were very happy to see it come back.
e

enough-analyst-54434

06/15/2021, 10:14 PM
Neither of your examples make sense. 1: How can a distribution work with 3.6+ but need to build a 3.8 specific .so? 2: If you have a dist with no python dependencies, but you do have a repo default IC how can you have an opinion of what version is setuptools to use? Isn't the latest compatible with the interpreter picked fine? Are you trying to concoct problems or are these real? They seem contrived and unreal.
h

hundreds-father-404

06/15/2021, 10:16 PM
I'm trying to solve the user's bug in a way that's consistent with how we handle other distributions (
pex_binary
and
python_awslambda
) and that is resilient to situations we can't fully anticipate, where it's not adequate to use
[python-setup].interpreter_constraints
to determine what to run setuptools with
h

helpful-lunch-92084

06/15/2021, 10:17 PM
yes, in v1 we heavily added interpreter_constraints to python_binary and python_test targets and left python_library targets largely without them (unless they needed to be explicitly set, e.g. they were a py3-only library and we didn’t want a cross project py2 dependency accidentally trying to use it). We also had many pex_binary targets that didn’t rely upon a library for like simple scripts. For example:
Copy code
pex_binary(
  name = "some_script",
  source="some_script.py",
  interpreter_constraints=["~3.6"]
)
or
Copy code
pex_binary(
    name="aws",
    entry_point="awscli.clidriver:main",
    dependencies=["3rdparty/python:awscli"],
    interpreter_constraints=["~3.6"],
)
however, i should add that the first pex_binary example is largely moot now that we can’t set a sources field on a pex_binary target so I had to convert those instances to:
Copy code
python_library(
  name = "some_script_lib",
  sources=["some_script.py"],
)

pex_binary(
  name = "some_script",
  entry_point = "some_script.py",
  dependencies=[":some_script_lib"],
)
In which case i could set an interpreter_constraint on the python_library
e

enough-analyst-54434

06/15/2021, 10:19 PM
So, the only situation in play is 0 python dependency distributions. Is that right?
Was that confirmed as the issue?
h

hundreds-father-404

06/15/2021, 10:23 PM
Two things going on: - User-reported bug when there are no deps on
python_library
targets, so no constraints used - Last month, Benjy and I realized it was a mistake to remove
interpreter_constraints
from
pex_binary
and
python_awslambda
. We fixed that. I've been pondering past month if we should update
python_distribution
to be consistent
e

enough-analyst-54434

06/15/2021, 10:26 PM
The 1st seems more pertinent. The second assumes consistency is a universal good. The question is why do pex_binaryband python_awslambda need ICs. I can't see a reason why python_distribution does.
Why does pex_binary need an IC? The only reason I can think of is Nate's answer which I thought was mooted in v2.
h

helpful-lunch-92084

06/15/2021, 10:28 PM
here is one reason for pex_binary
Copy code
pex_binary(
    name="aws",
    entry_point="awscli.clidriver:main",
    dependencies=["3rdparty/python:awscli"],
    interpreter_constraints=["~3.6"],
)
e

enough-analyst-54434

06/15/2021, 10:28 PM
Ah, yes. Ok, great. Good reason.
Why for awalambsa?
h

helpful-lunch-92084

06/15/2021, 10:28 PM
assume that awscli requires 3.6 but your default repo constraints are [2.7, 3.5+]
h

hundreds-father-404

06/15/2021, 10:28 PM
Gotta run to meeting but I tried to summarize convo in https://github.com/pantsbuild/pants/pull/12207