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.

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

Add pants_runtime_python_version global option #7363

Merged

Conversation

Projects
None yet
8 participants
@Eric-Arellano
Copy link
Contributor

commented Mar 12, 2019

Problem

As discussed in pantsbuild/setup#30, we decided to add support for using Python 3 to every day user's setup script ./pants by having them pin the interpreter version they want to use in pants.ini, and then grepping that value to choose the relevant interpreter. However, this currently results in this error:

ERROR] Invalid option 'pants_runtime_python_version' under [GLOBAL] in /Users/eric/DocsLocal/code/projects/setup/pants.ini
Exception caught: (<class 'pants.option.config.ConfigValidationError'>)

Exception message: Invalid config entries detected. See log for details on which entries to update or remove.
(Specify --no-verify-config to disable this check.)

Solution

Exactly how we handle --pants-version, introduce a new global option --pants-runtime-python-version that allows users to set pants_runtime_python_version in their pants.ini.

Even though Pants code cannot dynamically change the interpreter it runs with—as it is too late to change the interpreter once the process is already running—we can check that Pants is running with the intended interpreter and print a helpful message if it is not.

You can leave off the option, and you'll get no runtime check. Setup scripts are free to implement Python interpreter resolution in another way if they prefer.

Result

  • Including pants_runtime_python_version in pants.ini no longer results in an error.
  • Running ./pants --pants-runtime-python-version=2.6 will error out. Running ./pants2 --pants-runtime-python-version=2.7 will work normally.
@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

Reviewers: I added so many of you all because I wanted your feedback on the option name pants_engine_python_version. This is going to end up in almost everyone's pants.ini, just as python_version does, so it's important to get right.

Also this option is not going to go away once we drop Py2, as users will still be able to choose between 3.6 vs. 3.7.

The important thing to make sure we're communicating with this option name is that this does not impact the interpreter they use for their own code, e.g. in their tests and other spawned subprocesses. This solely determines what Pants itself uses to run.

--

Please refer to pantsbuild/setup#30 for the plan on how this will all hook up.

@stuhood
Copy link
Member

left a comment

Thanks!

Show resolved Hide resolved src/python/pants/option/global_options.py Outdated

Eric-Arellano added some commits Mar 12, 2019

Move comments directly into help argument
Danny and Stu have a point that it's better to expose this info directly to the user, rather than keeping it in a comment in source code.

I originally was thinking we'd want to keep the help message shorter, and rely on the message in the runtime check to steer people the right way, but this is bad reasoning. I can't think of a time I was upset a help message was too long (so long as it was concise and everything had a reason to be there)..

Eric-Arellano added a commit to Eric-Arellano/setup that referenced this pull request Mar 12, 2019

Remove pants_engine_python_version from pants.ini
We first need to land pantsbuild/pants#7363 and then release Pants with it or else the option will error out.
@illicitonion

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

Yay time to bikeshed!

My first preference would be --python-version; the pants is implicit.
My second preference would be --pants-python-version, if we're trying to explicitly differentiate from e.g. pytest-python-version. But I'd prefer --python-version :)

I also have a vague question around where we should be using exact versions and where we should be using constraints...

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

To recap, we now have multiple proposals for what to call the global option users will put in their pants.ini to pin the Python version they use to run Pants (not their own code).

  • python_version_for_pants
  • python_version_for_running_pants
  • pants_runtime_python_version
  • pants_engine_python_version
  • pants_native_python_version
  • pants_python_version
  • python_version

My main priority is distinguishing between options like python_setup_interpreter_constraints, to make crystal clear this interpreter version is only what Pants uses to run itself and is not what subprocesses like ./pants run and ./pants test will use.

Edit: added Benjy and Borja's proposals.

@benjyw

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

Does the word "runtime" help with this emphasis? E.g., pants_runtime_python_version?

@blorente

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

I do like the verboseness of that option and I wouldn't be afraid of being overly verbose, since this is a thing that is written down in a config file, not typed in a terminal.

I'm not sure I like the word runtime, because for someone who doesn’t know what runtime means or implies, that word means nothing (and they will have to ask regardless), and for someone who does know, they don’t need the clarification anyway. I speak from my own experience, but mileage my vary.

How about something like python_version_for_pants, or even python_version_for_running_pants?

Love a good bikeshedding session.

@OniOni

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

My two cents:
I like:

  • pants_engine_python_version
  • pants_runtime_python_version
    They're nice and explicit.
@CMLivingston

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

+1 for Mathieu's suggestion.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

Thank you all. We've narrowed it down to:

  • pants_engine_python_version
  • pants_runtime_python_version

Thoughts on which of the two?

My two cents, I'd be happy with either. I prefer pants_engine_python_version because the word engine suggests to me this is some "under the hood" thing Pants has to have and it's an implementation detail, rather than impacting their own code, whereas pants_runtime_python_version isn't quite as clear to me what runtime refers to (e.g. runtime meaning when you run ./pants test?). Either way, both are good.

@blorente

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

I prefer pants_engine_python_version. I can totally imagine myself being confused by it still, but that's as much bikeshedding as I'm willing to do :)

@illicitonion

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

I have a pretty strong preference away from engine - we use engine in other places to have a very specific meaning, and it's basically a pants-developer facing term that most users shouldn't never need to know exists.

@OniOni

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

I don't have strong feelings either way. But if engine does have a specific meaning in pants, I think that's enough to not use pants_engine_python_version.

So I would vote: pants_runtime_python_version

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

I like pants_runtime_python_version because it doesn't seem like this is purely relevant to the engine, which alone would make me want to use runtime (I would even be ok with pants_python_version if that wasn't itself very ambiguous). I also like the idea of keeping the engine concept more specific. I think pants_runtime_python_version is something users can see and possibly understand immediately even without looking at the help, but engine immediately makes it more mysterious, which doesn't seem like what we'd want for a setup script for someone adding pants to their repo for the first time (for example).

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

It looks like pants_runtime_python_version will work best! Thank you for pitching in everyone! I will make this update and merge after green CI 🎉

@Eric-Arellano Eric-Arellano changed the title Add --pants-engine-python-version global option Add pants_runtime_python_version global option Mar 13, 2019

Rename pants_engine_python_version to pants_runtime_python_version + …
…improve comments

Make explicit reference to the setup repo and our ./pants script we distribute. The average user will end up wanting to use this script, so it's good to remind them that we will parse these versions and resolve them automatically.

@Eric-Arellano Eric-Arellano merged commit d09c13a into pantsbuild:master Mar 13, 2019

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:pants-engine-python-version branch Mar 13, 2019

raise BuildConfigurationError(
'Running Pants with a different Python interpreter version than requested. '
'You requested {}, but are running with {}.\n\n'
'Note that Pants cannot use the value you give for `--pants-engine-python-version` to '

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 13, 2019

Member

This is using the previous option name.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 14, 2019

Author Contributor

Oof. I thought I had fixed all the instances of this. Will hotfix this as part of #7365. Thanks for the catch!

Eric-Arellano added a commit to Eric-Arellano/pants that referenced this pull request Mar 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.