Simplify PythonSetup usage #4328

Merged
merged 7 commits into from Mar 15, 2017

Conversation

Projects
None yet
2 participants
@benjyw
Contributor

benjyw commented Mar 14, 2017

(see d56240c for context).

  • Separates it from PythonRepos and moves it back into the
    python backend (but under the subsystems dir).

  • To do so, removes plugin_resover.py's usage of PythonSetup.
    The only use was for the resolver_cache_ttl option, and that
    option shouldn't really exist anyway (see below).

  • Deprecates the --resolver-cache-ttl option.
    There's no benefit to allowing re-resolution of requirements.
    If you're not content with "some version that satisfies a
    requirement, no matter how old" then you should tighten up your
    requirements... Re-resolving arbitrarily can only add to
    confusion and lack of repeatability, so now we simply hard-code
    a very large TTL (10 years) in the two places we need it (plus in
    python_chroot.py, but that will go away once we're on the new pipeine)
    and deprecate the option.

  • Centralizes all handling of interpreter selection constraints, which
    were previously spread across three options in three different scopes.
    This change moves the --constraints option from the new pipeline's
    interpreter selection task into PythonSetup, and deprecates the other
    two options.

Note that PythonRepos stays outside the python backend, but now
in its own file, as it is genuinely needed by the plugin_resolver.

@benjyw benjyw requested a review from kwlzn Mar 14, 2017

@benjyw benjyw changed the title from Move PythonSetup back into the python backend. to Simplify PythonSetup usage Mar 14, 2017

Simplify PythonSetup usage.
Separates it from PythonRepos and moves it back into the
python backend (but under the subsystems dir).

To do so, removes plugin_resover.py's usage of PythonSetup.
The only use was for the resolver_cache_ttl option, and that
option shouldn't really exist anyway (see below).

Deprecates the --resolver-cache-ttl option.
There's no benefit to allowing re-resolution of requirements.
If you're not content with "some version that satisfies a
requirement, no matter how old" then you should tighten up your
requirements... Re-resolving arbitrarily can only add to
confusion and lack of repeatability, so now we simply hard-code
a very large TTL (10 years) in the two places we need it (plus in
python_chroot.py, but that will go away once we're on the new pipeine)
and deprecate the option.

Centralizes all handling of interpreter selection constraints, which
were previously spread across three options in three different scopes.
This change moves the --constraints option from the new pipeline's
interpreter selection task into PythonSetup, and deprecates the other
two options.

Note that PythonRepos stays outside the python backend, but now
in its own file, as it is genuinely needed by the plugin_resolver.
@benjyw

This comment has been minimized.

Show comment
Hide comment
@benjyw

benjyw Mar 14, 2017

Contributor

Note that this is all prep work for rewriting the PythonEval task to use new pipeline constructs. It won't use the interpreter selected by the interpreter selection task (as that selects one for all targets in the closure, while PythonEval needs to select separately for each target it evals), so the --constraints option needs to be somewhere independent of either task, and PythonSetup is where all other interpreter selection options go.

We can move --constraints without deprecation because the new python pipeline has not appeared in any release.

Contributor

benjyw commented Mar 14, 2017

Note that this is all prep work for rewriting the PythonEval task to use new pipeline constructs. It won't use the interpreter selected by the interpreter selection task (as that selects one for all targets in the closure, while PythonEval needs to select separately for each target it evals), so the --constraints option needs to be somewhere independent of either task, and PythonSetup is where all other interpreter selection options go.

We can move --constraints without deprecation because the new python pipeline has not appeared in any release.

@kwlzn

lgtm, overall - but I'd like to see us keep the resolve cache ttl parameterizable via options as we sometimes rely on that as an escape hatch in the face of pex resolver bugs.

distributions[platform] = resolve(
requirements=[req.requirement for req in requirements],
interpreter=self._interpreter,
fetchers=fetchers,
platform=platform,
context=context,
cache=requirements_cache_dir,
- cache_ttl=self._python_setup.resolver_cache_ttl,
+ cache_ttl=10 * 365 * 86400,

This comment has been minimized.

@kwlzn

kwlzn Mar 14, 2017

Member

would prefer strongly to keep this parameterizable via options throughout.

@kwlzn

kwlzn Mar 14, 2017

Member

would prefer strongly to keep this parameterizable via options throughout.

This comment has been minimized.

@benjyw

benjyw Mar 14, 2017

Contributor

Would you be comfortable with this being parameterized when resolving user code (i.e., here and in pex_build_util.py) but not when resolving pants plugins themselves? That is supposed to be a one-time thing, and is a known, fixed universe, so the risk seems low? Or we could add a global option for this somewhere else. What I don't want is to have plugin_resolver.py reference PythonSetup (see above for more details on this).

@benjyw

benjyw Mar 14, 2017

Contributor

Would you be comfortable with this being parameterized when resolving user code (i.e., here and in pex_build_util.py) but not when resolving pants plugins themselves? That is supposed to be a one-time thing, and is a known, fixed universe, so the risk seems low? Or we could add a global option for this somewhere else. What I don't want is to have plugin_resolver.py reference PythonSetup (see above for more details on this).

@@ -41,6 +54,8 @@ def register_options(cls, register):
'If unspecified, a standard path under the workdir is used.')
register('--resolver-cache-ttl', advanced=True, type=int, metavar='<seconds>',
default=10 * 365 * 86400, # 10 years.
+ removal_version='1.5.0.dev0',
+ removal_hint='Defunct option. Resolver cache now never expires.',

This comment has been minimized.

@kwlzn

kwlzn Mar 14, 2017

Member

we actually use this occasionally as a workaround (e.g. ./pants --python-setup-resolver-cache-ttl=0 ...) to avoid use of the pex resolver cache as an alternative to a heavier cache dir removal/clean-all. it'd probably be a good idea to keep this around as an escape hatch for that purpose - at least until the pex resolver is 100% bug free.

@kwlzn

kwlzn Mar 14, 2017

Member

we actually use this occasionally as a workaround (e.g. ./pants --python-setup-resolver-cache-ttl=0 ...) to avoid use of the pex resolver cache as an alternative to a heavier cache dir removal/clean-all. it'd probably be a good idea to keep this around as an escape hatch for that purpose - at least until the pex resolver is 100% bug free.

from pants.backend.python.targets.python_target import PythonTarget
from pants.base.fingerprint_strategy import DefaultFingerprintHashingMixin, FingerprintStrategy
from pants.invalidation.cache_manager import VersionedTargetSet
-from pants.python.python_setup import PythonRepos, PythonSetup
+from pants.python.python_repos import PythonRepos

This comment has been minimized.

@kwlzn

kwlzn Mar 14, 2017

Member

is there a reason behind the import namespace split between PythonSetup vs PythonRepos? would generally expect them to live side-by-side at least in the same import namespace, no?

@kwlzn

kwlzn Mar 14, 2017

Member

is there a reason behind the import namespace split between PythonSetup vs PythonRepos? would generally expect them to live side-by-side at least in the same import namespace, no?

This comment has been minimized.

@benjyw

benjyw Mar 14, 2017

Contributor

Not necessarily. PythonRepos are needed both by the python backend and by core pants (in the plugin resolver), so they need to be in core Pants. And PythonRepos represents a clear concept that indeed has nothing specifically to do with the python backend.

PythonSetup, OTOH, is a bit of a hodge-podge of things, but most of them do relate specifically to the Python backend (e.g., how to set up an interpreter cache). So as long as the interpreter cache is in the Python backend and not the core, PythonSetup should be there too.

And indeed, the only thing the core needed from PythonSetup was that cache ttl setting, and that usage was pretty spurious - there's no reason to link "how pants resolves its plugins" with "how pants resolves code it builds".

@benjyw

benjyw Mar 14, 2017

Contributor

Not necessarily. PythonRepos are needed both by the python backend and by core pants (in the plugin resolver), so they need to be in core Pants. And PythonRepos represents a clear concept that indeed has nothing specifically to do with the python backend.

PythonSetup, OTOH, is a bit of a hodge-podge of things, but most of them do relate specifically to the Python backend (e.g., how to set up an interpreter cache). So as long as the interpreter cache is in the Python backend and not the core, PythonSetup should be there too.

And indeed, the only thing the core needed from PythonSetup was that cache ttl setting, and that usage was pretty spurious - there's no reason to link "how pants resolves its plugins" with "how pants resolves code it builds".

@benjyw

Thanks for the feedback! See my question below.

from pants.backend.python.targets.python_target import PythonTarget
from pants.base.fingerprint_strategy import DefaultFingerprintHashingMixin, FingerprintStrategy
from pants.invalidation.cache_manager import VersionedTargetSet
-from pants.python.python_setup import PythonRepos, PythonSetup
+from pants.python.python_repos import PythonRepos

This comment has been minimized.

@benjyw

benjyw Mar 14, 2017

Contributor

Not necessarily. PythonRepos are needed both by the python backend and by core pants (in the plugin resolver), so they need to be in core Pants. And PythonRepos represents a clear concept that indeed has nothing specifically to do with the python backend.

PythonSetup, OTOH, is a bit of a hodge-podge of things, but most of them do relate specifically to the Python backend (e.g., how to set up an interpreter cache). So as long as the interpreter cache is in the Python backend and not the core, PythonSetup should be there too.

And indeed, the only thing the core needed from PythonSetup was that cache ttl setting, and that usage was pretty spurious - there's no reason to link "how pants resolves its plugins" with "how pants resolves code it builds".

@benjyw

benjyw Mar 14, 2017

Contributor

Not necessarily. PythonRepos are needed both by the python backend and by core pants (in the plugin resolver), so they need to be in core Pants. And PythonRepos represents a clear concept that indeed has nothing specifically to do with the python backend.

PythonSetup, OTOH, is a bit of a hodge-podge of things, but most of them do relate specifically to the Python backend (e.g., how to set up an interpreter cache). So as long as the interpreter cache is in the Python backend and not the core, PythonSetup should be there too.

And indeed, the only thing the core needed from PythonSetup was that cache ttl setting, and that usage was pretty spurious - there's no reason to link "how pants resolves its plugins" with "how pants resolves code it builds".

distributions[platform] = resolve(
requirements=[req.requirement for req in requirements],
interpreter=self._interpreter,
fetchers=fetchers,
platform=platform,
context=context,
cache=requirements_cache_dir,
- cache_ttl=self._python_setup.resolver_cache_ttl,
+ cache_ttl=10 * 365 * 86400,

This comment has been minimized.

@benjyw

benjyw Mar 14, 2017

Contributor

Would you be comfortable with this being parameterized when resolving user code (i.e., here and in pex_build_util.py) but not when resolving pants plugins themselves? That is supposed to be a one-time thing, and is a known, fixed universe, so the risk seems low? Or we could add a global option for this somewhere else. What I don't want is to have plugin_resolver.py reference PythonSetup (see above for more details on this).

@benjyw

benjyw Mar 14, 2017

Contributor

Would you be comfortable with this being parameterized when resolving user code (i.e., here and in pex_build_util.py) but not when resolving pants plugins themselves? That is supposed to be a one-time thing, and is a known, fixed universe, so the risk seems low? Or we could add a global option for this somewhere else. What I don't want is to have plugin_resolver.py reference PythonSetup (see above for more details on this).

@kwlzn

This comment has been minimized.

Show comment
Hide comment
@kwlzn

kwlzn Mar 14, 2017

Member

Would you be comfortable with this being parameterized when resolving user code (i.e., here and in pex_build_util.py) but not when resolving pants plugins themselves?

sure, sgtm.

Member

kwlzn commented Mar 14, 2017

Would you be comfortable with this being parameterized when resolving user code (i.e., here and in pex_build_util.py) but not when resolving pants plugins themselves?

sure, sgtm.

@benjyw

This comment has been minimized.

Show comment
Hide comment
@benjyw

benjyw Mar 14, 2017

Contributor

Restored the cache ttl option as discussed.

Contributor

benjyw commented Mar 14, 2017

Restored the cache ttl option as discussed.

@benjyw

This comment has been minimized.

Show comment
Hide comment
@benjyw

benjyw Mar 14, 2017

Contributor

PTAL.

Contributor

benjyw commented Mar 14, 2017

PTAL.

@kwlzn

kwlzn approved these changes Mar 14, 2017

lgtm! thanks!

@benjyw benjyw merged commit 50544b9 into pantsbuild:master Mar 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjyw benjyw deleted the benjyw:move_python_setup branch Mar 15, 2017

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

Simplify PythonSetup usage (#4328)
Separates it from PythonRepos and moves it back into the
python backend (but under the subsystems dir).

To do so, removes plugin_resover.py's usage of PythonSetup.
The only use was for the resolver_cache_ttl option, which we
can hardcode.

Centralizes all handling of interpreter selection constraints, which
were previously spread across three options in three different scopes.
This change moves the --constraints option from the new pipeline's
interpreter selection task into PythonSetup, and deprecates the other
two options.

Note that PythonRepos stays outside the python backend, but now
in its own file, as it is genuinely needed by the plugin_resolver.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment