Remove direct pex imports from the codebase #21515
Conversation
krishnan-chandra
added
category:bugfix
| # 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 |
There was a problem hiding this comment.
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.
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Subtly changing the behavior of the test, no the plugin resovler itself. Correct?
There was a problem hiding this comment.
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.
krishnan-chandra
force-pushed
the
excise-direct-pex-imports
branch
from
d273ce6
to
5b5b8b7
Compare
krishnan-chandra
added
category:internal
|
Fixed the failing test - we can't remove |
| @@ -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 | |||
There was a problem hiding this comment.
Subtly changing the behavior of the test, no the plugin resovler itself. Correct?
| 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") |
There was a problem hiding this comment.
(I didn't know .pexrc files existed before looking at this change.)
| @@ -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"]}, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Closes #21401. This removes any direct imports of
pexin the codebase but retains it as a dependency, because certain tests depend on it.