Update pre-commit dependencies

[altheaden]

This PR updates the dependencies within .pre-commit-config.yaml to their most recent versions. It also adds pre-commit CI which will hopefully auto-update these dependencies on a monthly basis.

---

Updated template from @forsyth2:

Issue resolution

• Equivalent to Update pre-commit dependencies zstash#353

This pull request is a minor adjustment that does not affect functional code.

1. Does this do what we want it to do?

Objectives:

• Update the dependencies within .pre-commit-config.yaml to their most recent versions.
• Include the pre-commit code stylistic changes introduced by this.
• Add pre-commit CI which will hopefully auto-update these dependencies on a monthly basis.

Required:

• Product Management: I have confirmed with the stakeholders that the objectives above are correct and complete.
• Testing: I have added or modified at least one "min-case" configuration file to test this change. Every objective above is represented in at least one cfg.
• Changes to functional code are minimal. Excluding formatting changes, the only changes are from type() == to isinstance. These changes are unlikely to create errors as noted in Update pre-commit dependencies #627 (comment). In any case, the unit tests are passing and the weekly integration tests will be run on main after merging.
• Testing: I have considered likely and/or severe edge cases and have included them in testing.

2. Are the implementation details accurate & efficient?

Required:

• Logic: I have visually inspected the entire pull request myself.

3. Is this well documented?

Required:

• Documentation: by looking at the docs, a new user could easily understand the functionality introduced by this pull request.
  • This change does not require new documentation.

4. Is this code clean?

Required:

• Readability: The code is as simple as possible and well-commented, such that a new team member could understand what's happening.
• Pre-commit checks: All the pre-commits checks have passed.

[xylar]

I'll note that we don't really know if this will do anything useful. It is discussed on the web as a potential equivalent of dependabot for pre-commit but I suspect we may need to do something more than add this chunk of code to get it to actually run.

[xylar]

@altheaden, looks good!

@forsyth2, I don't have permission to approve the workflow to let CI run. Could you do that?

[forsyth2]

I don't have permission to approve the workflow to let CI run. Could you do that?

Just ran it.

Like #620 & #621, I'll merge this once we get the tests on main sorted out (#625)

[xylar]

Thanks @forsyth2. It looks like you all have things configure to lint all files, not just the ones changed in a given PR. You might want to change that unless you want folks to have to clean up the mess that other folks left.

Here's the bit that does that in mache:
https://github.com/E3SM-Project/mache/blob/main/.github/workflows/build_workflow.yml#L38-L51

@forsyth2, is that also a change you'd like to have. If so, I think @altheaden would also be willing to do a PR for that, right @altheaden?

[xylar]

Maybe in combination with black, that doesn't make sense -- you want to lint all the files all the time. Maybe we just need to fix all the files that black is flagging in a PR as soon as this goes in.

[altheaden]

I'm happy to either help out with implementing the change @xylar suggested or to help clean up the formatting of any older files, whichever would be more helpful.

[forsyth2]

lint all files, not just the ones changed in a given PR. You might want to change that unless you want folks to have to clean up the mess that other folks left.

Personally, I don't really have a problem with linting all files. Checking all files means the entire repo meets our pre-commit checks. And if for some reason, a --no-verify commit got merged in, we could make another commit to main to address that specifically (as to not "pollute" a new PR). In theory, main meets all the checks, so any pre-commit failures would have necessarily been introduced by the latest change.

[xylar]

@forsyth2, okay. How do you want to handle the lint getting flagged here? Fix it in this PR or open a separate one?

[forsyth2]

How do you want to handle the lint getting flagged here?

I don't understand how this is happening.

On the latest main:

pre-commit run --all-files
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check yaml...............................................................Passed
black....................................................................Passed
isort....................................................................Passed
flake8...................................................................Passed
mypy.....................................................................Passed

https://github.com/E3SM-Project/zppy/actions/runs/11150511889/job/30992064405?pr=627 shows:

trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check yaml...............................................................Passed
black....................................................................Failed
- hook id: black
- files were modified by this hook

reformatted setup.py
reformatted zppy/e3sm_diags.py
reformatted zppy/templates/coupled_global.py

All done! ✨ 🍰 ✨
3 files reformatted, 22 files left unchanged.

Were these automated reformattings launched because of the pre-commit changes in this PR? In that case, it would make sense to include the formatting changes as part of this PR.

If not, then I'm not really sure why there would be pre-commit formatting differences

[altheaden]

I wonder if some default behavior in black changed between its current release version and the one currently on zppy. Maybe it's checking something by default and we need to add a flag of some kind to change that behavior. I'll investigate more tomorrow, it's definitely very weird that it's checking all these files for no reason. (Unless this ends up being the desired behavior - in that case I'll see what changes it wants and I can add them to this PR if that's desired as well)

[xylar]

I agree with @altheaden that the update in black is the most likely reason. Let's include the linting changes in this PR. They do look more correct.

[altheaden]

@xylar @forsyth2 I've updated the formatting, some of which was automatic via pre-commit and some of which was manual.

The manual changes I made were to change statements of the form:

type(<obj>) == <type>

to:

isinstance(<obj>, <type>)

which was one of two possible fixes suggested by flake8. isinstance(x, y) compares the types of x and y while accounting for inheritance, as opposed to only checking the types of x and y directly to find if they are an identical match.

The alternative fix suggested by flake8 was to compare objects using the is operator instead of the == operator, but that doesn't seem like what we want in this case (as I understand it, comparing with is checks if the two objects point to the same place in memory, i.e., they are the exact same object).

Without understanding the whole context of the code, I am not sure if these changes are desired or not. If we want to ignore inheritance and check only the two types directly, then I can revert these changes and set flake8 to ignore them (or figure out a better alternative).

These changes will definitely need to be tested and I'm not totally sure how to do that for zppy. I looked through the documentation but I'm not certain what I should use for the config file if I'm going to be testing these changes specifically.

[forsyth2]

If we want to ignore inheritance and check only the two types directly

We did run into an issue with isinstance on that front before -- see #538 (comment). That said, I see isinstance appear 5 times in this PR, 4 of which are base types like bool or str. The only one I could see being an issue is the Section one, but I don't feel like that would be an issue (and if it was, it would easily show up running the unit tests).

[forsyth2]

These changes will definitely need to be tested and I'm not totally sure how to do that for zppy.

zppy is quite a challenge to test properly -- we have to properly align runtime environments for various different tasks, check that each task is successful (effectively that means zppy adds another layer of testing to all the packages it coordinates), and choose important subsets of parameters to test (since trying to test everything amounts to combinatorial explosion). I've tried to document & automate this very-involved process as much as I can, but challenges remain.

Some resources:

• https://docs.e3sm.org/zppy/_build/html/main/dev_guide/release_testing.html
• https://github.com/E3SM-Project/zppy/blob/main/tests/integration/generated/directions_chrysalis.md

I usually run what I call min_case cfg files to test specific changes. For these sort of overarching DevOps changes that propagate to all/many pieces of the code, really just running the Weekly tests would be the best option.

I can do that for this PR after I get the current tests passing again (#625).

I'm not certain what I should use for the config file

For changes in the yml files, I take extra care to create a new conda environment from the updated yml, and then use that environment for integration testing.

[altheaden]

@forsyth2 I think the reason CI failed for this second run is because the pre-commit job is timing out in the caching step - once #621 gets merged this timeout is upped to 5 mins to avoid this problem. I think if you re-run it again it'll be faster due to caching and be able to complete, that's been my experience with the same issue in other repos.

[forsyth2]

@altheaden yep looks like it passes now!

[altheaden]

Just updated this to a slightly newer version of these dependencies than when I initially made this PR. I also added changes to the versions in conda/dev.yml. I did my best guess for some (such as the version for flake8-isort which wasn't obvious if it should be updated for the most recent flake8, the most recent isort, or something else).

@tomvothecoder since you mentioned this file to me on Slack, do you mind taking a look at my updates to conda/dev.yml to make sure I am updating things in the way you expect?

[altheaden]

@forsyth2 Just rebased this branch onto main and fixed the merge conflicts, so it's ready to move forward into testing whenever.

[tomvothecoder]

@tomvothecoder since you mentioned this file to me on Slack, do you mind taking a look at my updates to conda/dev.yml to make sure I am updating things in the way you expect?

Looks good to me. Thank you @altheaden.

[altheaden]

@forsyth2 looks like CI passed for everything other than pre-commit timing out at the caching step again.

[xylar]

@forsyth2 and @altheaden, thanks again!

[forsyth2]

Hmm, @altheaden after merging this, I'm getting this error over on #623 after trying to run our pre-commit checks.

$ pre-commit run --all-files
[INFO] Initializing environment for https://github.com/pre-commit/pre-commit-hooks.
An error has occurred: InvalidManifestError: 
==> File /gpfs/fs1/home/ac.forsyth2/.cache/pre-commit/repo0641jows/.pre-commit-hooks.yaml
==> At Hook(id='check-added-large-files')
==> At key: stages
==> At index 0
=====> Expected one of commit, commit-msg, manual, merge-commit, post-checkout, post-commit, post-merge, post-rewrite, prepare-commit-msg, push but got: 'pre-commit'
Check the log at /gpfs/fs1/home/ac.forsyth2/.cache/pre-commit/pre-commit.log

Is there a different command now?

[xylar]

@forsyth2, maybe try removing ~/.cache/pre-commit, and then maybe try doing pre-commit install again just in case that helps? You do have the new zppy_dev environment active, rigth?

[xylar]

I just successfully ran:

 git clone [email protected]:E3SM-Project/zppy.git main
 cd main
 conda env create -y -n zppy_dev -f conda/dev.yml 
 conda activate zppy_dev
 pre-commit run --all-files

As expected, I see:

[WARNING] top-level `default_stages` uses deprecated stage names (commit) which will be removed in a future version.  run: `pre-commit migrate-config` to automatically fix this.                                                               
[INFO] Initializing environment for https://github.com/pre-commit/pre-commit-hooks.                                                                             
[INFO] Initializing environment for https://github.com/psf/black.               
[INFO] Initializing environment for https://github.com/PyCQA/isort.             
[WARNING] repo `https://github.com/PyCQA/isort` uses deprecated stage names (commit, merge-commit, push) which will be removed in a future version.  Hint: often `pre-commit autoupdate --repo https://github.com/PyCQA/isort` will fix this.  if it does not -- consider reporting an issue to that repo.
[INFO] Initializing environment for https://github.com/pycqa/flake8.
[INFO] Initializing environment for https://github.com/pycqa/flake8:flake8-isort==6.1.1.
[INFO] Initializing environment for https://github.com/pre-commit/mirrors-mypy.
[INFO] Installing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/psf/black.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/PyCQA/isort.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/pycqa/flake8.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/pre-commit/mirrors-mypy.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check yaml...............................................................Passed
black....................................................................Passed
isort....................................................................Passed
flake8...................................................................Passed
mypy.....................................................................Passed

[xylar]

(It looks like there are 2 deprecation warnings to fix still.)

[forsyth2]

You do have the new zppy_dev environment active

Ah, of course. That completely slipped my mind, thanks. It's working now with a dev environment built off the latest main.

(It looks like there are 2 deprecation warnings to fix still.)

Yes, I'm seeing that too.