Pants no longer respects flagged interpreter constraints in repl.py task

[CMLivingston]

Previously, ./pants clean-all repl --python-setup-interpreter-constraints="['>3']" <target> on a 2/3-comaptible target (such as >=2.7,<4) would select an appropriate interpreter for the repl session in accordance with the supplied constraints. In this case, we would repl with Python 3.

As of 1.15.0rc0+git5c01b701, this is no longer the case, and the option parsing for the repl task completely ignores the flagged option and goes straight to the config default in pants.ini.

To reproduce, checkout a fresh master and run:
./pants clean-all repl --python-setup-interpreter-constraints="['CPython>=3.6']" testprojects/src/python/interpreter_selection:echo_interpreter_version_lib
This should create a repl with Python 3 as indicated by the constraints applied on the cmdline.

During my initial digging, it looks like PythonSetup.global_instance().interpreter_constraints is failing to load the correct value from the cmdline flag and instead contains pants.ini defaults, or the hardcoded default. In interpreter_cache.py and python_execution_task_base.py, calls to compatibility_or_constraints fail to trigger interpreter_constraints as a "flagged" option, and instead proceeding as if we had not passed it on the command line. This first manifests during interpreter selection, and then again when we build a repl pex with the same constraints in python_execution_task_base.py.

---

And, for reference:

$ ./pants --python-setup-interpreter-constraints="['>3']" options | grep 'python.setup.interpreter.constraints'
python-setup.interpreter_constraints = ['>3'] (from FLAG in pants.ini)

[CMLivingston]

From my initial investigation, it appears that this is not taking place during interpreter selection, and likely manifests earlier during option parsing, as self._python_setup.interpreter_constraints is the pants.ini value and not the flagged value when we setup the cache in interpreter_cache.py.

[CMLivingston]

I did some old fashion manual bisecting and found bc4df78 to be functioning properly. This is likely not the earliest functional commit, but I started walking up from when I added the functionality to accomplish this with the repl task.

EDIT: looks like da18eee (release 1.13.0) is working as well.

[jsirois]

It does! But this is a scope issue and you're not going to like it:

As you reported the issue, ie: .. repl --python-setup-interpreter-constraints=... - which is repl scoped (which is not even repl.py scoped - but neither here nor there because of below...):

jsirois@gill ~/dev/pantsbuild/pants ((5c01b7019...)) $ ./pants clean-all options --name=interpreter-constraints repl --python-setup-interpreter-constraints="['==2.7.*']" src/python/pants/base:deprecated
INFO] For async removal, run `./pants clean-all --async`
conan.interpreter_constraints = ['CPython>=2.7,<4'] (from HARDCODED)
grpcio.interpreter_constraints = [] (from HARDCODED)
isort.interpreter_constraints = [] (from HARDCODED)
lambdex.interpreter_constraints = [] (from HARDCODED)
python-setup.interpreter_constraints = ['CPython==3.7.3'] (from ENVIRONMENT in pants.ini, from env var PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS)
python-setup.export.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.lint.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.lint.python-eval.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.mypy.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.pyprep.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.pyprep.requirements.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.repl.interpreter_constraints = ['==2.7.*'] (from FLAG)
python-setup.repl.py.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.run.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.run.py.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.test.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.test.pytest-prep.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)

Python 3.7.3 (default, Mar 26 2019, 21:43:19) 
[GCC 8.2.1 20181127] on linux
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> 
now exiting InteractiveConsole...

Using the global scope, ie: ./pants --python-setup-interpreter-constraints=... repl ...:

jsirois@gill ~/dev/pantsbuild/pants ((5c01b7019...)) $ ./pants clean-all options --name=interpreter-constraints --python-setup-interpreter-constraints="['==2.7.*']" repl src/python/pants/base:deprecated
INFO] For async removal, run `./pants clean-all --async`
conan.interpreter_constraints = ['CPython>=2.7,<4'] (from HARDCODED)
grpcio.interpreter_constraints = [] (from HARDCODED)
isort.interpreter_constraints = [] (from HARDCODED)
lambdex.interpreter_constraints = [] (from HARDCODED)
python-setup.interpreter_constraints = ['==2.7.*'] (from FLAG in pants.ini, from env var PANTS_PYTHON_SETUP_INTERPRETER_CONSTRAINTS)
python-setup.export.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.lint.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.lint.python-eval.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.mypy.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.pyprep.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.pyprep.requirements.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.repl.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.repl.py.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.run.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.run.py.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.test.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)
python-setup.test.pytest-prep.interpreter_constraints = ['CPython>=2.7,<3', 'CPython>=3.6,<4'] (from HARDCODED)

Python 2.7.16 (default, Mar 11 2019, 18:59:25) 
[GCC 8.2.1 20181127] on linux2
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> 

So you can still tell repl.py to use the flagged interpreter constraints - but it looks at the Pythonetup global instance - not the goal or task scoped one.

I'm going to close this as answered, but if the scope did change without deprecation - this is a legitimate bug. If you want to pursue the scope change bug and have Pants supply backwards compatibility for the goal (and task?) scopes, that may be worth a new more targetd issue.

[CMLivingston]

Thanks, John, for the clarification! I will look into whether this was not deprecated properly and follow up.

[CMLivingston]

Any ideas of where/when this took place? I take it somewhere a subsystem dependency got changed from global -> scoped, but seems like this it affects more than just the interpreter cache.

[jsirois]

Nope, I leave that spelunking to you. This is eminently git bisectable...

[CMLivingston]

No problem, I was just trying to see if you have any quick thoughts because the bisect + repro required a cargo rebuild for most of the attempts. Finally found it:

Author: Daniel Wagner-Hall <dawagner@gmail.com>
Date:   Wed Feb 6 10:47:30 2019 +0000

    Resolve all platforms from all python targets (#7156)

So I am clear here, the framing of a new ticket would be to reinstate the old behavior and add a deprecation warning for 2 minor version releases?

[CMLivingston]

Hmm, looks like we might have a bug that was introduced by #7156. Namely the difference between PythonSetup as a scoped subsystem dependency at https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/python/tasks/resolve_requirements_task_base.py#L41 vs the global property at https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/python/tasks/resolve_requirements_task_base.py#L50. Was this intended to be a global or a scoped instance of PythonSetup @illicitonion ? If global, I can make a quick fix, and if scoped, I'll look into adding a deprecation warning, although I have to admit I would need a nudge in the right direction in terms of how to do that (conditional scoping of subsystems maybe?).

[illicitonion]

Oops, sorry about that!

I don't really know much about subsystem scopes. My initial instinct is that it should probably be scoped to the task, because the task does the resolve and is the thing that cares about platforms. But it kind of feels like this kind of platform constraints specification is a global decision that cuts across multiple tasks, so maybe it should just be a global config that everything uses.

So what I guess I'm saying is, I'd like to defer to someone who understands the pants optioning system better than I do.

[CMLivingston]

The precedent in the codebase is to use the global instance when using PythonSetup, and I think global makes sense given that constraints should apply to all tasks in order to align with interpreter selection + the interpreter product. Allowing different constraints for each task feels like an anti-pattern to me, or a highly specialized use case. Given that detail and the codebase consistency aspect, I think global makes sense. Thoughts here @jsirois ?

[Eric-Arellano]

Agreed that as a user I would anticipate interpreter constraints to apply to all tasks. If that wasn’t the case, I would expect the option name to reflect that it’s just for that task, eg —repl-interpreter-constraints.

It sounds like there wasn’t a specific use case that we were designing for in making it task specific.

[benjyw]

Yeah, I think using the global option makes sense. Having different interpreter constraints for different tasks on the same set of targets is not a particularly useful ability. It's almost never what the user intends, and definitely not worth the confusion.

[benjyw]

@Eric-Arellano The full option name does reflect the task scope: --repl-py-python-setup-interpreter-constraints, but we have this "feature" that allows you to omit the task scope when you place the flag after the task name on the cmd line.

This was something that Twitter (and I think, specifically, Brian Wickman) wanted. I'm not sure it's worth it though. I'd be in favor of killing it and requiring fully-scoped flags always.

[stuhood]

There are definitely folks internally who dislike the ability to use short flag names in context. I like it, because it allows things like:

./pants publish.jar --no-commit --snapshot --etc --furthermore

...to be ~10x shorter.

But it definitely causes some confusion... and it's possible that in the context of v2, using the abbreviated form would become significantly less useful, since goals will no longer have subscopes.

[illicitonion]

There are definitely folks internally who dislike the ability to use short flag names in context. I like it, because it allows things like:

./pants publish.jar --no-commit --snapshot --etc --furthermore

...to be ~10x shorter.

But it definitely causes some confusion... and it's possible that in the context of v2, using the abbreviated form would become significantly less useful, since goals will no longer have subscopes.

I'm pretty strongly against the abbreviation thing, and would love to get rid of it if we can :)

[CMLivingston]

Closed by #7672.