pantsbuild / pants

The Pants Build System
https://www.pantsbuild.org
Apache License 2.0
3.21k stars 620 forks source link

Read Interpreter Constraints from `.python-version` pyenv file #18730

Open rhuanbarreto opened 1 year ago

rhuanbarreto commented 1 year ago

Is your feature request related to a problem? Please describe.

Today we set the python version to work with our pants enabled repo by using the pyenv .python-version file. Every time we update our python version in the file, we need to remember to update the [python].interpreter_constraints to ["==new_version"] in pants.toml or leave the constraint open like [">=3.11"]

Describe the solution you'd like

Accepting a value like "<PYENV_LOCAL>" as pants does in [python-bootstrap].search_path would simplify this process.

Describe alternatives you've considered

Today the process can only be made manually.

kaos commented 1 year ago

Today the process can only be made manually.

There are work-arounds available.

# .pants.bootstrap
export PANTS_PYTHON_INTERPRETER_CONSTRAINTS===$(pyenv version-name)

This will add to the constraints. To set it to just that single value:

# .pants.bootstrap
export PANTS_PYTHON_INTERPRETER_CONSTRAINTS="[\"==$(pyenv version-name)\"]"

Verify with:

$ pants python --help-advanced
[...]
  --python-interpreter-constraints="[<requirement>, <requirement>, ...]"
  PANTS_PYTHON_INTERPRETER_CONSTRAINTS
  interpreter_constraints
      default: []
      current value: [
          "==3.9.13"
      ] (from env var PANTS_PYTHON_INTERPRETER_CONSTRAINTS)
          overrode: [
              ">=3.7,<3.10"
          ] (from pants.toml)
      The Python interpreters your codebase is compatible with.
[...]
rhuanbarreto commented 1 year ago

Thanks for showing one possible solution!

/.pants.* files are in gitignore, so should I commit this then? Would it affect other processes, like CI?

kaos commented 1 year ago

You may commit it, and yes if present it would affect CI as well. Using scie-pants there's also an option to use a .env file, although I have not first hand experience with that.

rhuanbarreto commented 1 year ago

But then it can be a problem because in CI, python comes from PATH, and in development it comes from PYENV_LOCAL.

Is there a way to tweak your workaround so if $CI == true then PANTS_PYTHON_INTERPRETER_CONSTRAINTS is $(python --version) else it's "[\"==$(pyenv version-name)\"]"

jsirois commented 1 year ago

@rhuanbarreto the .pants.bootstrap script is evaluated in bash; so yes. You can use any bash logic you desire to setup the final exported env vars Pants will read.

rhuanbarreto commented 1 year ago

Thanks all for the feedback. But still the feature request would make this easier so I could use PYENV_LOCAL on pants.toml

kaos commented 1 year ago

Thanks all for the feedback. But still the feature request would make this easier so I could use PYENV_LOCAL on pants.toml

For sure. Not trying to take away your feature request, merely presenting a viable workaround for comparison with the requested feature (and potentially use until/if implemented):

I would do as little as possible, which could be:

[python]
interpreter_constraints = ["==3.10.0"]
# .pants.bootstrap
[ "$CI" == "true" ] || { 
  # only override the interpreter version when not running in CI
  export PANTS_PYTHON_INTERPRETER_CONSTRAINTS="[\"==$(pyenv version-name)\"]"
}

An issue I see with the feature request is that if your .python-version file is checked in, how do we know to not consult it during CI when you have <PYENV_LOCAL> in your pants.toml?

One answer could be to put it in pants.ci.toml, but then that's not much easier than the workaround, imho. Thoughts?

rhuanbarreto commented 1 year ago

An issue I see with the feature request is that if your .python-version file is checked in, how do we know to not consult it during CI when you have in your pants.toml?

When I setup Python in CI I ask to consult the file to load the version defined there in the Python version file. So there's only one source of truth ruling the version of the Python interpreter.

For sure this only covers the case where the monorepo uses only one Python version for all of it. But it's still very handy to avoid updating Python versions all over the repo and instead leveraging dependency management tools like dependabot or renovate to keep things in sync.

kaos commented 1 year ago

An issue I see with the feature request is that if your .python-version file is checked in, how do we know to not consult it during CI when you have in your pants.toml?

When I setup Python in CI I ask to consult the file to load the version defined there in the Python version file. So there's only one source of truth ruling the version of the Python interpreter.

Then why would this be an issue?

But then it can be a problem because in CI, python comes from PATH, and in development it comes from PYENV_LOCAL.

benjyw commented 10 months ago

I'm not convinced we want this feature. To set the ICs based on a file that is traditionally gitignored, and that can vary from system to system, seems scary. After all, there is no guarantee that Pants will use that pyenv interpreter on a given system. It depends on config on that system that is outside Pants's control. You yourself give the example of this needing to be different in CI vs desktops. And for this to even work on desktops you would need tighter control over them than you probably have.

We have been moving in the opposite direction - towards Pants installing hermetic Python for you, based on the ICs. I think that is the more productive direction to move to. See https://github.com/pantsbuild/pants/pull/18352 (I don't think there's user documentation yet, right @thejcannon ?)

benjyw commented 10 months ago

In other words, "this code is compatible with these interpreter versions" is a property of the code, and so should live in metadata alongside the code - in this case pants.toml.

It is not a property of the current state of the system Pants is currently being run on. So the metadata architecture should reflect that.

thejcannon commented 10 months ago

I see the use-case for @rhuanbarreto for sure, since in this case the ICs and the python version represent different things, but share a link. If you're only ever testing your repo using Python 3.N, you wouldn't wanna claim you support 3.M because that's lower.

That being said, I think leveraging the experimental "Python Providers" is the way to unify this. @rhuanbarreto can you see if that's a direction that'd work for you? Otherwise, I, personally, would be OK re-opening and continuing the conversation.

rhuanbarreto commented 10 months ago

@thejcannon even though having a hermetic python would also help a lot, tools like renovatebot or dependabot don't support reading the interpreter constraints inside of a pants.toml file. So having a config saying to respect the python-version file would be very much beneficial.

For example, the setup-python GitHub action respects it so I don't need to specify again in yaml if I change the value on the file.

@benjyw "this code is compatible with these interpreter versions" is a property of the code but code is not a standalone entity. All the devops around the code also depends on this property and the industry standard today is to have a python-version file in order to have an unified way for handling both in CI and in dependency management tools.

Today managing this is a devops hassle as there's no simple way to unify this without doing some shell script magic.

So I would vote for having this feature still because it makes the experience of the infra/devops engineer way easier to maintain. Today I live with the fact that I need to manually update all references to python whenever a new version comes in a renovate PR.

rhuanbarreto commented 10 months ago

Same goes for Node.js that Pants will keep pushing forward. The .nvmrc file is the source of truth for the Node.js version and the official setup node action also reads the version from it.

benjyw commented 10 months ago

That's interesting that the setup-python action respects .python-version. That changes my opinion somewhat - it sounds like checking that file in is a thing people do. It's still only weak enforcement (you have to be using pyenv shims) but at least CI will use it consistently.

benjyw commented 10 months ago

So given that, I think supporting a <PYENV_LOCAL> magic string makes sense.