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

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

Open
rhuanbarreto opened this issue Apr 13, 2023 · 16 comments
Open

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

rhuanbarreto opened this issue Apr 13, 2023 · 16 comments
Labels
backend: Python Python backend-related issues enhancement

Comments

@rhuanbarreto
Copy link
Contributor

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
Copy link
Member

kaos commented Apr 13, 2023

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
Copy link
Contributor Author

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
Copy link
Member

kaos commented Apr 14, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor

jsirois commented Apr 17, 2023

@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
Copy link
Contributor Author

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

@kaos
Copy link
Member

kaos commented Apr 18, 2023

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?

@thejcannon thejcannon added the backend: Python Python backend-related issues label May 9, 2023
@rhuanbarreto
Copy link
Contributor Author

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?

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
Copy link
Member

kaos commented Jun 26, 2023

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?

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
Copy link
Contributor

benjyw commented Sep 22, 2023

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 #18352 (I don't think there's user documentation yet, right @thejcannon ?)

@benjyw
Copy link
Contributor

benjyw commented Sep 22, 2023

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.

@benjyw benjyw closed this as not planned Won't fix, can't repro, duplicate, stale Sep 22, 2023
@thejcannon
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor

benjyw commented Sep 28, 2023

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 benjyw reopened this Sep 28, 2023
@benjyw
Copy link
Contributor

benjyw commented Sep 28, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: Python Python backend-related issues enhancement
Projects
None yet
Development

No branches or pull requests

5 participants