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

Jump to bottom

Remove direct pex imports from the codebase #21515

Merged
merged 3 commits into from
Oct 17, 2024
Merged

Remove direct pex imports from the codebase #21515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a099433
Remove direct pex imports from the codebase
krishnan-chandra Oct 10, 2024
6610943
Adjust the version constraints on patch versions in test
krishnan-chandra Oct 10, 2024
5b5b8b7
Add dependency on pex executable in test
krishnan-chandra Oct 14, 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
2 changes: 1 addition & 1 deletion src/python/pants/backend/python/util_rules/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ python_tests(
name="tests",
overrides={
"local_dists_test.py": {"timeout": 120},
"pex_from_targets_test.py": {"timeout": 200},
"pex_from_targets_test.py": {"timeout": 200, "dependencies": ["3rdparty/python#pex"]},
Copy link
Contributor

Choose a reason for hiding this comment

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

Not as part of this PR, but after seeing the errors, thoughts on the LoE to remove the test dependency? Eventually I think we will want to consider using the scie Pex distributions instead of the PyPI wheels.

Copy link
Contributor Author

@krishnan-chandra krishnan-chandra Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think the main thing that is required to get these tests working without the dependency is finding a way to get pex inside the sandbox without explicitly requiring it. We may be able to potentially find another way to do that, but that's the crux issue for why these tests fail.

I'm not opposed to doing it as part of this PR, I just need to figure out HOW to do it.

"pex_test.py": {"timeout": 600, "dependencies": [":complete_platform_pex_test"]},
"package_dists_test.py": {"timeout": 150},
"vcs_versioning_test.py": {"timeout": 120},
Expand Down
43 changes: 39 additions & 4 deletions src/python/pants/core/subsystems/python_bootstrap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,9 @@
import itertools
import logging
import os
import sys
from dataclasses import dataclass
from typing import Collection

from pex.variables import Variables
from typing import Collection, Optional

from pants.core.util_rules import asdf, search_paths
from pants.core.util_rules.asdf import AsdfPathString, AsdfToolPathsResult
Expand Down Expand Up @@ -226,13 +225,49 @@ async def _expand_interpreter_search_paths(
return _SearchPaths(tuple(expanded))


# This method is copied from the pex package, located at pex.variables.Variables._get_kv().
# It is copied here to avoid a hard dependency on pex.
def _get_kv(variable: str) -> Optional[list[str]]:
kv = variable.strip().split("=")
if len(list(filter(None, kv))) == 2:
return kv
else:
return None


# This method is copied from the pex package, located at pex.variables.Variables.from_rc().
# It is copied here to avoid a hard dependency on pex.
def _read_pex_rc(rc: Optional[str] = None) -> dict[str, str]:
"""Read pex runtime configuration variables from a pexrc file.

:param rc: an absolute path to a pexrc file.
:return: A dict of key value pairs found in processed pexrc files.
"""
ret_vars = {}
rc_locations = [
os.path.join(os.sep, "etc", "pexrc"),
os.path.join("~", ".pexrc"),
os.path.join(os.path.dirname(sys.argv[0]), ".pexrc"),
]
if rc:
rc_locations.append(rc)
for filename in rc_locations:
try:
with open(os.path.expanduser(filename)) as fh:
rc_items = map(_get_kv, fh)
ret_vars.update(dict(filter(None, rc_items)))
except OSError:
continue
return ret_vars
Comment on lines +228 to +261
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This involves reading the pexrc file, which kind of explains why we were directly importing from pex before. I'm not sure if it's worth keeping a hard dependency just for this, but maybe it is in case that logic changes in the future.



def _get_pex_python_paths():
"""Returns a list of paths to Python interpreters as defined in a pexrc file.

These are provided by a PEX_PYTHON_PATH in either of '/etc/pexrc', '~/.pexrc'. PEX_PYTHON_PATH
defines a colon-separated list of paths to interpreters that a pex can be built and run against.
"""
ppp = Variables.from_rc().get("PEX_PYTHON_PATH")
ppp = _read_pex_rc().get("PEX_PYTHON_PATH")
Copy link
Contributor

Choose a reason for hiding this comment

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

(I didn't know .pexrc files existed before looking at this change.)

if ppp:
return ppp.split(os.pathsep)
else:
Expand Down
15 changes: 5 additions & 10 deletions tests/python/pants_test/init/test_plugin_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
from typing import Dict, Iterable, Sequence

import pytest
from pex.interpreter import PythonInterpreter
from pkg_resources import Distribution, Requirement, WorkingSet

from pants.backend.python.util_rules import pex
Expand All @@ -31,7 +30,6 @@
from pants.testutil.python_interpreter_selection import (
PY_38,
PY_39,
python_interpreter_path,
skip_unless_python38_and_python39_present,
)
from pants.testutil.rule_runner import EXECUTOR, QueryRule, RuleRunner
Expand Down Expand Up @@ -125,7 +123,7 @@ class Plugin:
def plugin_resolution(
rule_runner: RuleRunner,
*,
interpreter: PythonInterpreter | None = None,
python_version: str | None = None,
chroot: str | None = None,
plugins: Sequence[Plugin] = (),
requirements: Iterable[str] = (),
Expand All @@ -143,7 +141,7 @@ def provide_chroot(existing):

# Default to resolving with whatever we're currently running with.
interpreter_constraints = (
InterpreterConstraints([f"=={interpreter.identity.version_str}"]) if interpreter else None
InterpreterConstraints([f"=={python_version}.*"]) if python_version else None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note - this does subtly change because we are no longer using Pex to hard-pin the patch version of Python in this constraint; we only pin the minor version. That could potentially be a problem if this behavior is particularly material to the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Subtly changing the behavior of the test, no the plugin resovler itself. Correct?

Copy link
Contributor Author

@krishnan-chandra krishnan-chandra Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Correct, behavior of the test. Before, the interpreter constraints for the test would be targeting a specific patch version (inferred from the environment) rather than a minor version.

)
artifact_interpreter_constraints = interpreter_constraints or InterpreterConstraints(
[f"=={'.'.join(map(str, sys.version_info[:3]))}"]
Expand Down Expand Up @@ -301,12 +299,9 @@ def test_exact_requirements_interpreter_change_bdist(rule_runner: RuleRunner) ->


def _do_test_exact_requirements_interpreter_change(rule_runner: RuleRunner, sdist: bool) -> None:
python38 = PythonInterpreter.from_binary(python_interpreter_path(PY_38))
python39 = PythonInterpreter.from_binary(python_interpreter_path(PY_39))

with plugin_resolution(
rule_runner,
interpreter=python38,
python_version=PY_38,
plugins=[Plugin("jake", "1.2.3"), Plugin("jane", "3.4.5")],
sdist=sdist,
) as results:
Expand All @@ -316,7 +311,7 @@ def _do_test_exact_requirements_interpreter_change(rule_runner: RuleRunner, sdis
with pytest.raises(ExecutionError):
with plugin_resolution(
rule_runner,
interpreter=python39,
python_version=PY_39,
chroot=chroot,
plugins=[Plugin("jake", "1.2.3"), Plugin("jane", "3.4.5")],
):
Expand All @@ -333,7 +328,7 @@ def _do_test_exact_requirements_interpreter_change(rule_runner: RuleRunner, sdis
# directly from the still in-tact cache.
with plugin_resolution(
rule_runner,
interpreter=python38,
python_version=PY_38,
chroot=chroot,
plugins=[Plugin("jake", "1.2.3"), Plugin("jane", "3.4.5")],
) as results2:
Expand Down
Loading