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.

Sign up for GitHub

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

Plumb Pip's --{no,only}-binary. #2346

Merged
merged 7 commits into from
Jan 26, 2024
Merged

Plumb Pip's --{no,only}-binary. #2346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
111cfae
Plumb Pip's `--{no,only}-binary`.
jsirois Jan 26, 2024
3257bbb
@kaos's wording fix.
jsirois Jan 26, 2024
e924df4
Fix `--use-pep517` help PEP URL.
jsirois Jan 26, 2024
c00afb7
DRY up BuildConfiguration access.
jsirois Jan 26, 2024
6aa4ea4
Prepare the 2.1.161 release.
jsirois Jan 26, 2024
a1f0aad
Fix import formatting.
jsirois Jan 26, 2024
992c544
Fix test verbage expectation.
jsirois Jan 26, 2024
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
10 changes: 10 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
# Release Notes

## 2.1.161

This release adds support for `--only-wheel <project name>` and
`--only-build <project name>` to allow finer control over which
distribution artifacts are resolved when building a PEX or creating or
updating a lock file. These options correspond to Pip's `--only-binary`
and `--no-binary` options with project name arguments.

* Plumb Pip's `--{no,only}-binary`. (#2346)

## 2.1.160

This release adds the ability for `pex3 lock update` to replace
Expand Down
8 changes: 2 additions & 6 deletions pex/cli/commands/lock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,9 +591,7 @@ def _export(self, requirement_configuration=RequirementConfiguration()):
lock=lock_file,
requirement_configuration=requirement_configuration,
network_configuration=network_configuration,
build=lock_file.allow_builds,
use_wheel=lock_file.allow_wheels,
prefer_older_binary=lock_file.prefer_older_binary,
build_configuration=lock_file.build_configuration(),
transitive=lock_file.transitive,
include_all_matches=True,
)
Expand Down Expand Up @@ -733,9 +731,7 @@ def _update(self):
targets=targets,
lock=lock_file,
network_configuration=network_configuration,
build=lock_file.allow_builds,
use_wheel=lock_file.allow_wheels,
prefer_older_binary=lock_file.prefer_older_binary,
build_configuration=lock_file.build_configuration(),
transitive=lock_file.transitive,
)
)
Expand Down
94 changes: 49 additions & 45 deletions pex/pip/tool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,11 @@
from pex.pip.tailer import Tailer
from pex.pip.version import PipVersion, PipVersionValue
from pex.platforms import Platform
from pex.resolve.resolver_configuration import ReposConfiguration, ResolverVersion
from pex.resolve.resolver_configuration import (
BuildConfiguration,
ReposConfiguration,
ResolverVersion,
)
from pex.targets import LocalInterpreter, Target
from pex.tracer import TRACER
from pex.typing import TYPE_CHECKING
Expand Down Expand Up @@ -401,6 +405,36 @@ def _spawn_pip_isolated_job(
)
return Job(command=command, process=process, finalizer=finalizer)

@staticmethod
def _iter_build_configuration_options(build_configuration):
# type: (BuildConfiguration) -> Iterator[str]

# N.B.: BuildConfiguration maintains invariants that ensure --only-binary, --no-binary,
# --prefer-binary, --use-pep517 and --no-build-isolation are coherent.

if not build_configuration.allow_builds:
yield "--only-binary"
yield ":all:"
elif not build_configuration.allow_wheels:
yield "--no-binary"
yield ":all:"
else:
for project in build_configuration.only_wheels:
yield "--only-binary"
yield str(project)
for project in build_configuration.only_builds:
yield "--no-binary"
yield str(project)

if build_configuration.prefer_older_binary:
yield "--prefer-binary"

if build_configuration.use_pep517 is not None:
yield "--use-pep517" if build_configuration.use_pep517 else "--no-use-pep517"
Comment on lines +432 to +433
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: what makes the pep517 option different from the other bool options?

(realized I could probably reply to myself here by reading pip install --help)
A: the difference aiui is that pep517 is used by default, so the option really is about disabling it.

Copy link
Member Author

@jsirois jsirois Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for digging there, but I'm still crying a bit. Did you check out Pex's own docs?:
https://github.com/pantsbuild/pex/blob/015a077d3496205824491bf97136afd719277b12/pex/resolve/resolver_options.py#L206-L224

Assuming those are read, if they still don't make sense, it would be good to know so I can fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed that bad URL.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, my bad for not realizing the option was of course documented for pex as well (got tunnel visioned onto the pip arg..).

Yea, the pex option has a lot of information to digest. Reading it through a few times makes it almost clear. I wonder if there is a typo here:

---  If PEP-517 is forced (--use-pep517 is passed)
+++  If PEP-517 is forced (--force-pep517 is passed)

then again, I would expect that --force-pep517 is the same as --use-pep517 (based on my understanding of how ArgumentParser works), but the help has so many layers that I get the impression that they are not.. (also, that it reads default: None rather than default: True doesn't help which would align with the help text stating that it uses pep517 by default)
So, I feel there's a bit of language ... (failed to find the word I was looking for) here.

I think this option would be clearer using an enum value, but I guess the current state is due to upholding backwards compatibility..? In which case I think making the relationship between --force-pep517 and --use-pep517 clearer would help, and perhaps also mentioning that the default behavior is based on not providing a value for the option (e.g. explain that this flag is a tri-bool)

I can see why you cry a bit over this.

Copy link
Member Author

@jsirois jsirois Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, so to sum up - I think you figured this out, but in case not:

  • The option is a None|True|False tri-state
  • The default is None
  • The option names are all just synonyms (with --no- prefixes negating)

So the default, None, means do the right thing: if there is a pyproject.toml build system, use it; if not use setup.py.

True means use (force) pep 517 always (concoct a setuptools legacy build backend as needed).

False means do not use pep 517.

I have no clue what use cases benefit from the True or False cases, certainly the False case makes no sense I can see. I just exposed this Pip knob to not get between users and Pip. Perhaps my sin was in trying to make the Pip help different / more clear in up in Pex. I clearly failed there!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks. Yea, that summation aligns with my current understanding.


if not build_configuration.build_isolation:
yield "--no-build-isolation"

def spawn_download_distributions(
self,
download_dir, # type: str
Expand All @@ -411,47 +445,25 @@ def spawn_download_distributions(
transitive=True, # type: bool
target=None, # type: Optional[Target]
package_index_configuration=None, # type: Optional[PackageIndexConfiguration]
build=True, # type: bool
use_wheel=True, # type: bool
prefer_older_binary=False, # type: bool
use_pep517=None, # type: Optional[bool]
build_isolation=True, # type: bool
build_configuration=BuildConfiguration(), # type: BuildConfiguration
observer=None, # type: Optional[DownloadObserver]
preserve_log=False, # type: bool
):
# type: (...) -> Job
target = target or targets.current()

if not use_wheel:
if not build:
raise ValueError(
"Cannot both ignore wheels (use_wheel=False) and refrain from building "
"distributions (build=False)."
)
elif not isinstance(target, LocalInterpreter):
raise ValueError(
"Cannot ignore wheels (use_wheel=False) when resolving for a platform: "
"{}".format(target.platform)
)
if not build_configuration.allow_wheels and not isinstance(target, LocalInterpreter):
raise ValueError(
"Cannot ignore wheels (use_wheel=False) when resolving for a platform: "
"{}".format(target.platform)
)

download_cmd = ["download", "--dest", download_dir]
extra_env = {} # type: Dict[str, str]
pex_extra_sys_path = [] # type: List[str]

if not build:
download_cmd.extend(["--only-binary", ":all:"])

if not use_wheel:
download_cmd.extend(["--no-binary", ":all:"])

if prefer_older_binary:
download_cmd.append("--prefer-binary")

if use_pep517 is not None:
download_cmd.append("--use-pep517" if use_pep517 else "--no-use-pep517")

if not build_isolation:
download_cmd.append("--no-build-isolation")
download_cmd.extend(self._iter_build_configuration_options(build_configuration))
if not build_configuration.build_isolation:
pex_extra_sys_path.extend(sys.path)

if allow_prereleases:
Expand Down Expand Up @@ -589,7 +601,7 @@ def _ensure_wheel_installed(self, package_index_configuration=None):
download_dir=atomic_dir.work_dir,
requirements=[self._version.wheel_requirement],
package_index_configuration=package_index_configuration,
build=False,
build_configuration=BuildConfiguration.create(allow_builds=False),
).wait()
for wheel in glob.glob(os.path.join(atomic_dir.work_dir, "*.whl")):
install_wheel_interpreter(wheel_path=wheel, interpreter=pip_interpreter)
Expand All @@ -600,9 +612,7 @@ def spawn_build_wheels(
wheel_dir, # type: str
interpreter=None, # type: Optional[PythonInterpreter]
package_index_configuration=None, # type: Optional[PackageIndexConfiguration]
prefer_older_binary=False, # type: bool
use_pep517=None, # type: Optional[bool]
build_isolation=True, # type: bool
build_configuration=BuildConfiguration(), # type: BuildConfiguration
verify=True, # type: bool
):
# type: (...) -> Job
Expand All @@ -613,16 +623,10 @@ def spawn_build_wheels(
wheel_cmd = ["wheel", "--no-deps", "--wheel-dir", wheel_dir]
extra_env = {} # type: Dict[str, str]

# It's not clear if Pip's implementation of PEP-517 builds respects this option for
# resolving build dependencies, but in case it is we pass it.
if use_pep517 is not False and prefer_older_binary:
wheel_cmd.append("--prefer-binary")

if use_pep517 is not None:
wheel_cmd.append("--use-pep517" if use_pep517 else "--no-use-pep517")

if not build_isolation:
wheel_cmd.append("--no-build-isolation")
# It's not clear if Pip's implementation of PEP-517 builds respects all build configuration
# options for resolving build dependencies, but in case it does, we pass them all.
wheel_cmd.extend(self._iter_build_configuration_options(build_configuration))
if not build_configuration.build_isolation:
interpreter = interpreter or PythonInterpreter.get()
extra_env.update(PEX_EXTRA_SYS_PATH=os.pathsep.join(interpreter.sys_path))

Expand Down
12 changes: 2 additions & 10 deletions pex/resolve/configured_resolve.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,7 @@ def resolve(
resolver_version=pip_configuration.resolver_version,
network_configuration=pip_configuration.network_configuration,
password_entries=pip_configuration.repos_configuration.password_entries,
build=pip_configuration.allow_builds,
use_wheel=pip_configuration.allow_wheels,
prefer_older_binary=pip_configuration.prefer_older_binary,
use_pep517=pip_configuration.use_pep517,
build_isolation=pip_configuration.build_isolation,
build_configuration=pip_configuration.build_configuration,
compile=compile_pyc,
max_parallel_jobs=pip_configuration.max_jobs,
pip_version=lock.pip_version,
Expand Down Expand Up @@ -95,11 +91,7 @@ def resolve(
resolver_version=resolver_configuration.resolver_version,
network_configuration=resolver_configuration.network_configuration,
password_entries=resolver_configuration.repos_configuration.password_entries,
build=resolver_configuration.allow_builds,
use_wheel=resolver_configuration.allow_wheels,
prefer_older_binary=resolver_configuration.prefer_older_binary,
use_pep517=resolver_configuration.use_pep517,
build_isolation=resolver_configuration.build_isolation,
build_configuration=resolver_configuration.build_configuration,
compile=compile_pyc,
max_parallel_jobs=resolver_configuration.max_jobs,
ignore_errors=ignore_errors,
Expand Down
12 changes: 2 additions & 10 deletions pex/resolve/configured_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,7 @@ def resolve_lock(
find_links=self.pip_configuration.repos_configuration.find_links,
resolver_version=self.pip_configuration.resolver_version,
network_configuration=self.pip_configuration.network_configuration,
build=self.pip_configuration.allow_builds,
use_wheel=self.pip_configuration.allow_wheels,
prefer_older_binary=self.pip_configuration.prefer_older_binary,
use_pep517=self.pip_configuration.use_pep517,
build_isolation=self.pip_configuration.build_isolation,
build_configuration=self.pip_configuration.build_configuration,
compile=False,
transitive=self.pip_configuration.transitive,
verify_wheels=True,
Expand Down Expand Up @@ -95,11 +91,7 @@ def resolve_requirements(
find_links=self.pip_configuration.repos_configuration.find_links,
resolver_version=self.pip_configuration.resolver_version,
network_configuration=self.pip_configuration.network_configuration,
build=self.pip_configuration.allow_builds,
use_wheel=self.pip_configuration.allow_wheels,
prefer_older_binary=self.pip_configuration.prefer_older_binary,
use_pep517=self.pip_configuration.use_pep517,
build_isolation=self.pip_configuration.build_isolation,
build_configuration=self.pip_configuration.build_configuration,
compile=False,
max_parallel_jobs=self.pip_configuration.max_jobs,
ignore_errors=False,
Expand Down
36 changes: 14 additions & 22 deletions pex/resolve/lock_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
from pex.resolve.lockfile.model import Lockfile
from pex.resolve.lockfile.subset import subset
from pex.resolve.requirement_configuration import RequirementConfiguration
from pex.resolve.resolver_configuration import ResolverVersion
from pex.resolve.resolver_configuration import BuildConfiguration, ResolverVersion
from pex.resolve.resolvers import MAX_PARALLEL_DOWNLOADS, Resolver, ResolveResult
from pex.resolver import BuildAndInstallRequest, BuildRequest, InstallRequest
from pex.result import Error, catch, try_
Expand All @@ -44,7 +44,11 @@
if TYPE_CHECKING:
from typing import Dict, Iterable, Mapping, Optional, Sequence, Tuple, Union

import attr # vendor:skip

from pex.hashing import HintedDigest
else:
from pex.third_party import attr


class FileArtifactDownloadManager(DownloadManager[FileArtifact]):
Expand Down Expand Up @@ -81,8 +85,7 @@ def __init__(
network_configuration=None, # type: Optional[NetworkConfiguration]
password_entries=(), # type: Iterable[PasswordEntry]
cache=None, # type: Optional[str]
use_pep517=None, # type: Optional[bool]
build_isolation=True, # type: bool
build_configuration=BuildConfiguration(), # type: BuildConfiguration
pex_root=None, # type: Optional[str]
pip_version=None, # type: Optional[PipVersionValue]
resolver=None, # type: Optional[Resolver]
Expand All @@ -98,8 +101,9 @@ def __init__(
self._network_configuration = network_configuration
self._password_entries = password_entries
self._cache = cache
self._use_pep517 = use_pep517
self._build_isolation = build_isolation
self._build_configuration = attr.evolve(
build_configuration, allow_wheels=False, prefer_older_binary=False
Copy link
Member Author

@jsirois jsirois Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is a bug. The incoming BuildConfiguration from the user may say allow_builds=False, allow_wheels=True (the known useful example being a user constraining a --style universal lock to just wheels), in which case just flipping allow_wheels=False here leads immediately to BuildConfiguration failing a pre-condition check (allow_wheels=False, allow_builds=False makes no sense and is illegal). The VCSArtifactDownloadManager should complete its thought and say it uses the user BuildConfiguration but trumps allow_wheels=False, allow_builds=True since the VCSArtifactDownloadManager must be able to build a wheel from the VCS checkout to do anything useful at all.

See #2389 for the fallout.

)
self._pip_version = pip_version
self._resolver = resolver
self._use_pip_config = use_pip_config
Expand All @@ -123,10 +127,7 @@ def save(
resolver_version=self._resolver_version,
network_configuration=self._network_configuration,
password_entries=self._password_entries,
use_wheel=False,
prefer_older_binary=False,
use_pep517=self._use_pep517,
build_isolation=self._build_isolation,
build_configuration=self._build_configuration,
max_parallel_jobs=1,
pip_version=self._pip_version,
resolver=self._resolver,
Expand Down Expand Up @@ -237,11 +238,7 @@ def resolve_from_lock(
resolver_version=None, # type: Optional[ResolverVersion.Value]
network_configuration=None, # type: Optional[NetworkConfiguration]
password_entries=(), # type: Iterable[PasswordEntry]
build=True, # type: bool
use_wheel=True, # type: bool
prefer_older_binary=False, # type: bool
use_pep517=None, # type: Optional[bool]
build_isolation=True, # type: bool
build_configuration=BuildConfiguration(), # type: BuildConfiguration
compile=False, # type: bool
transitive=True, # type: bool
verify_wheels=True, # type: bool
Expand All @@ -262,9 +259,7 @@ def resolve_from_lock(
constraint_files=constraint_files,
),
network_configuration=network_configuration,
build=build,
use_wheel=use_wheel,
prefer_older_binary=prefer_older_binary,
build_configuration=build_configuration,
transitive=transitive,
)
)
Expand Down Expand Up @@ -311,8 +306,7 @@ def resolve_from_lock(
resolver_version=resolver_version,
network_configuration=network_configuration,
password_entries=password_entries,
use_pep517=use_pep517,
build_isolation=build_isolation,
build_configuration=build_configuration,
pip_version=pip_version,
resolver=resolver,
use_pip_config=use_pip_config,
Expand Down Expand Up @@ -435,9 +429,7 @@ def resolve_from_lock(
use_pip_config=use_pip_config,
),
compile=compile,
prefer_older_binary=prefer_older_binary,
use_pep517=use_pep517,
build_isolation=build_isolation,
build_configuration=build_configuration,
verify_wheels=verify_wheels,
pip_version=pip_version,
resolver=resolver,
Expand Down
Loading
Loading