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

DEV - Set a canonical default Python version (3.12) #844

Merged

Conversation

trallard
Copy link
Collaborator

@trallard trallard commented Jul 4, 2024

This is another stack to #838 and intended to be merged after #841

Description

While reviewing #841, @peytondmurray suggested bumping the default Python version used for CI, Docker images, and the like.

This PR bumps us from 3.10 to 3.12:

  • Adds a .python-version-default file to serve as the canonical source of truth across all the places where we set Python versions
  • Updates ./github/workflows to use this new file to get the canonical Python version
  • Bumps the Python version in other places

Pull request checklist

  • Did you test this change locally?
  • Did you update the documentation (if required)?
  • Did you add/update relevant tests for this change (if required)?

Additional information

How to test

@trallard trallard requested a review from jaimergp July 4, 2024 12:07
@trallard trallard requested a review from peytondmurray July 4, 2024 12:07
Copy link
Contributor

@peytondmurray peytondmurray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if there was any other way to solve this, but this seems reasonable.

The only ones I could find that were missed are the entries in pyproject.toml. Do we need to worry about those?

Also the tests are failing, but I think that's from the previous PR that got merged.

@jaimergp
Copy link
Member

jaimergp commented Jul 5, 2024

This is a better way of consolidating the information. My two cents:

  • There's no standard filename or location for this type of file, but I do see some projects using this filename. There are others like simply .python-version (no -default).
  • Parsing from a plain text file might introduce accidental trailing whitespace. Usually tricky to debug. We could make sure that the file is always whitespace stripped with a pre-commit rule for that file only. Or if there's none, a simply Python step in the workflow:
- name: Make sure python-version has no whitespace
  shell: python
  run: |
    from pathlib import Path
    assert Path(".python-version-default").read_text().strip() == Path(".python-version-default").read_text()
  • The environment.yml file python version might get out of date. There's a comment about it but if someone updates the python-version-default file they might not see the environment.yml one. So I suggest we add a little check step like above:
- name: Make sure python-version has no whitespace
  shell: python
  run: |
    from pathlib import Path
    assert f'- python={Path(".python-version-default").read_text().strip()}' in Path("environment.yml").read_text()

@peytondmurray
Copy link
Contributor

Yep, now that you mention it pre-commit isn't hitting this file with the current configuration. Can we change it to strip trailing whitespace everywhere? I don't actually know of a single situation in which I'd want trailing whitespace.

  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v4.6.0
    hooks:
      - id: trailing-whitespace
        files: ".*\\.py"
      - id: check-added-large-files
      - id: check-toml
      - id: end-of-file-fixer
      - id: trailing-whitespace

@jaimergp
Copy link
Member

jaimergp commented Jul 5, 2024

And maybe make sure end-of-file-fixer doesn't add the extra newline?

@trallard
Copy link
Collaborator Author

trallard commented Jul 8, 2024

Also the tests are failing, but I think that's from the previous PR that got merged.

@peytondmurray No this is the same original error in my PR about the pip key rather than from the previous PR

The environment.yml file python version might get out of date. There's a comment about it but if someone updates the python-version-default file they might not see the environment.yml one. So I suggest we add a little check step like above:

My goal is to eventually just get rid of the environment.yaml files; right now, the only one that might be going stale is the dev-environment.yaml file, as we are otherwise using the python-default-version file to set the Python version elsewhere. So for practical purposes the Python version would always be correct, at least on CI.

Now if someone uses that locally then that might be out of date 🤷🏽‍♀️ but I see the proposed workflow as already more robust sans the end of file/whitespace thingy.

@trallard
Copy link
Collaborator Author

trallard commented Jul 8, 2024

The only ones I could find that were missed are the entries in pyproject.toml. Do we need to worry about those?

Why would we need to change these? These are the Python requires 3.8 so unless we are dropping 3.8-11 we do not need to change these

@trallard
Copy link
Collaborator Author

trallard commented Jul 8, 2024

I fixed the pre-commit hooks, so there should be no need to add extra steps.

There's no standard filename or location for this type of file, but I do see some projects using this filename. There are others, such as simply .python-version (no default).

Since there is no standard, I used python-version-default for symmetry with what we were using in some GH workflows (env variable), but I am not privy to keeping or changing the name in case either of you have a preference @jaimergp @peytondmurray

@peytondmurray
Copy link
Contributor

I'm totally fine with .python-version-default, and the changes .pre-commit-config.yml look good to me. if @jaimergp is okay with it, let's merge. 🚀

@trallard
Copy link
Collaborator Author

trallard commented Jul 8, 2024

Ok will merge.

@trallard trallard merged commit e8956f4 into trallard/update-dev-environments Jul 8, 2024
12 of 13 checks passed
@trallard trallard deleted the trallard/default-python-version branch July 8, 2024 18:32
@peytondmurray
Copy link
Contributor

Closes #842.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done 💪🏾
Development

Successfully merging this pull request may close these issues.

3 participants