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

use explicit Pex options for package "binary-ness" #20598

Merged
merged 2 commits into from
Feb 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions src/python/pants/backend/python/goals/lockfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,18 +79,6 @@ async def _setup_pip_args_and_constraints_file(resolve_name: str) -> _PipArgsAnd
args = list(resolve_config.pex_args())
digests = []

if resolve_config.no_binary or resolve_config.only_binary:
pip_args_file = "__pip_args.txt"
args.extend(["-r", pip_args_file])
pip_args_file_content = "\n".join(
[f"--no-binary {pkg}" for pkg in resolve_config.no_binary]
+ [f"--only-binary {pkg}" for pkg in resolve_config.only_binary]
)
pip_args_digest = await Get(
Digest, CreateDigest([FileContent(pip_args_file, pip_args_file_content.encode())])
)
digests.append(pip_args_digest)

if resolve_config.constraints_file:
args.append(f"--constraints={resolve_config.constraints_file.path}")
digests.append(resolve_config.constraints_file.digest)
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/backend/python/util_rules/pex_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class PexCli(TemplatedExternalTool):

default_version = "v2.1.163"
default_url_template = "https://github.com/pex-tool/pex/releases/download/{version}/pex"
version_constraints = ">=2.1.148,<3.0"
version_constraints = ">=2.1.161,<3.0"

@classproperty
def default_known_versions(cls):
Expand Down
27 changes: 25 additions & 2 deletions src/python/pants/backend/python/util_rules/pex_requirements.py
Original file line number Diff line number Diff line change
Expand Up @@ -349,8 +349,7 @@ class ResolvePexConfig:
def pex_args(self) -> Iterator[str]:
"""Arguments for Pex for indexes/--find-links, manylinux, and path mappings.

Does not include arguments for constraints files, --only-binary, and --no-binary, which must
be set up independently.
Does not include arguments for constraints files, which must be set up independently.
"""
# NB: In setting `--no-pypi`, we rely on the default value of `[python-repos].indexes`
# including PyPI, which will override `--no-pypi` and result in using PyPI in the default
Expand All @@ -367,6 +366,30 @@ def pex_args(self) -> Iterator[str]:
else:
yield "--no-manylinux"

# Pex logically plumbs through equivalent settings, but uses a
# separate flag instead of the Pip magic :all:/:none: syntax. To
# support the exitings Pants config settings we need to go from
# :all:/:none: --> Pex options, which Pex will translate back into Pip
# options. Note that Pex's --wheel (for example) means "allow
# wheels", not "require wheels".
if self.only_binary and ":all:" in self.only_binary:
yield "--wheel"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are several available synonyms available from Pex here.

yield "--no-build"
elif self.only_binary and ":none:" in self.only_binary:
yield "--no-wheel"
yield "--build"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I leaned into "explicit is better than implicit" for these "redundant" pairs.

elif self.only_binary:
yield from (f"--only-binary={pkg}" for pkg in self.only_binary)

if self.no_binary and ":all:" in self.no_binary:
yield "--no-wheel"
yield "--build"
elif self.no_binary and ":none:" in self.no_binary:
yield "--wheel"
yield "--no-build"
elif self.no_binary:
yield from (f"--only-build={pkg}" for pkg in self.no_binary)

yield from (f"--path-mapping={v}" for v in self.path_mappings)


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

from __future__ import annotations

import itertools
import json
import textwrap

Expand Down Expand Up @@ -347,3 +348,78 @@ def test_pex_lockfile_requirement_count() -> None:
)
== 3
)


class TestResolvePexConfigPexArgs:
Copy link
Contributor

Choose a reason for hiding this comment

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

Pants doesn't usually use these sort of classes to group tests, instead of just top-level test functions. What's the thinking with this approach here?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. This is remnant of a unittest style era, not all tests have been refactored to the pytest style yet, so those old ones that still exist shouldn't be used as role models :) c.f. #13208

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a little confused here. Python is an object oriented language, Pants does not otherwise eschew objects, and grouping tests with classes is a common idiom pytest has right in the getting started guide.

It's all dicts and functions somewhere down there to Python, you can totally do OOP-y stuff without "objects" as such (Hello C!), and the silicon doesn't care. So if Pants has a style guide that has some reason for not using objects -- or no reason! that's fine too! -- I'd know how to adjust. But in this case I did it this way because an idiomatic way to test the methods of a class is a test class with methods.

Copy link
Contributor

@huonw huonw Feb 28, 2024

Choose a reason for hiding this comment

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

Ah sorry, I'm not sure it's explicitly covered by https://www.pantsbuild.org/2.20/docs/contributions/development/style-guide#tests, given it falls somewhere between the two examples there (not unittest, but also not covered by the "Good" example), but class-less seems to be the implicit style. I don't think it's worth blocking this PR, but @kaos is the old timer with more of the history here so his vote is stronger 😄

I'll wait until tomorrow and get it in if we don't hear (given how small it is).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right. yea the current style is class-less, but I see what you mean. (at first I failed to observe this was new tests altogether..)

I'm OK with using a bare class (i.e. not a TestCase derived one) to group related tests under. We should however update the style guide accordingly to cover this regardless of outcome.

Perhaps take a vote in #development slack?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm gonna merge this. It sounds like this is perfectly fine for pytest, and it's a bit silly to block a PR on the basis of something small like this.

@kaos I don't feel strongly enough about this to spend my energy defining the style here. I assume you will do that if you want to? But, if you do so and the result is that we need to come back and change this, I'm happy to do it (since I'm the one doing the merge).

Copy link
Member

Choose a reason for hiding this comment

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

perhaps not very clear, but I was in favour of allowing this style for tests, and agree we need not hold this PR for the sake of it. I can run a little poll to formalize the style docs.

def pairwise(self, iterable):
# Drop once on 3.10
# https://docs.python.org/3/library/itertools.html#itertools.pairwise
a, b = itertools.tee(iterable)
next(b, None)
return zip(a, b)

def simple_config_args(self, manylinux=None, only_binary=None, no_binary=None):
return tuple(
ResolvePexConfig(
indexes=[],
find_links=[],
manylinux=manylinux,
constraints_file=None,
no_binary=FrozenOrderedSet(no_binary) if no_binary else FrozenOrderedSet(),
only_binary=FrozenOrderedSet(only_binary) if only_binary else FrozenOrderedSet(),
path_mappings=[],
).pex_args()
)

def test_minimal(self):
args = self.simple_config_args()
assert len(args) == 2

def test_manylinux(self):
assert "--no-manylinux" in self.simple_config_args()

many = "manylinux2014_ppc64le"
args = self.simple_config_args(manylinux=many)
assert len(args) == 3
assert ("--manylinux", many) in self.pairwise(args)

def test_only_binary(self):
assert "--only-binary=foo" in self.simple_config_args(only_binary=["foo"])
assert ("--only-binary=foo", "--only-binary=bar") in self.pairwise(
self.simple_config_args(only_binary=["foo", "bar"])
)

def test_only_binary_all(self):
args = self.simple_config_args(only_binary=[":all:"])
assert "--wheel" in args
assert "--no-build" in args
assert "--only-binary" not in " ".join(args)

args = self.simple_config_args(only_binary=["foo", ":all:"])
assert "--wheel" in args
assert "--no-build" in args
assert "--only-binary" not in " ".join(args)

def test_only_binary_none(self):
assert "--wheel" not in self.simple_config_args(only_binary=[":none:"])
assert "--only-binary" not in " ".join(self.simple_config_args(only_binary=[":none:"]))
assert "--build" in self.simple_config_args(only_binary=[":none:"])

def test_no_binary(self):
assert "--only-build=foo" in self.simple_config_args(no_binary=["foo"])
assert ("--only-build=foo", "--only-build=bar") in self.pairwise(
self.simple_config_args(no_binary=["foo", "bar"])
)

def test_no_binary_all(self):
args = self.simple_config_args(no_binary=[":all:"])
assert "--build" in args
assert "--no-wheel" in args
assert "--no-binary" not in args

def test_no_binary_none(self):
assert "--wheel" in self.simple_config_args(no_binary=[":none:"])
assert "--only-build" not in " ".join(self.simple_config_args(no_binary=[":none:"]))

assert "--wheel" in self.simple_config_args(no_binary=["foo", ":none:"])
assert "--only-build" not in " ".join(self.simple_config_args(no_binary=["foo", ":none:"]))
Loading