Issue 66: simplify and split CI actions

[SamuelBrand1]

This is a PR for simplifying the CI in forecasttools-py. It closes #66 .

This PR splits rendering the README away from pre-commit and makes the pre-commit more minimalistic, following pyrenew-hew.

The main upsides of this change:

1. Is that subtle changes in the rendered README is no longer cause CI errors as per Update numpyro to 0.17 #64 .
2. My view is that render operations relying on the code should live in documentation rather than the README but YMMV here I guess.
3. As per 2, this splits the CI into more workflows each doing a simpler thing each.

The main downside of this change is that any change to the README has to be committed and isn't part of the pre-commit process. The new workflow just checks that this process doesn't fail.

[AFg6K7h4fhy2]

• Thank you for these contributions.
• W.r.t. the README rendering edit, I would like @dylanhmorris to be the output node here.
• I do not fully believe in the pre-existing workflow, but also do not know how long this suggested workflow will last, though I am tentatively open to deferring to this suggested workflow (pending comments from DHM).
• Beyond the ruff hook, I do not see why the pre-commit file needs to be simplified, generally speaking. The additional hooks removed have been helpful at times albeit, for some of them, quite infrequently.
• I still need to check on my end if the numpyro upgrade causes CI errors.

There following is a modified ruff hook I've put together that is not yet on main (some quotes come from ruffs website).

-   repo: https://github.com/astral-sh/ruff-pre-commit
    rev: v0.9.4
    hooks:
    # "currently, the Ruff formatter does not sort imports.
    # In order to both sort imports and format, call
    # the Ruff linter and then the formatter:"
    -   id: ruff
        args: [
          "check",
          "--select",
          # isort
          "I",
          "--fix"]
    # run ruff linter; the Ruff Linter is an extremely fast Python linter
    # designed as a drop-in replacement for Flake8 (plus dozens of plugins),
    # isort, pydocstyle, pyupgrade, autoflake, and more
    -   id: ruff
        args: [
          # ambiguous variable name: {name}
          "--ignore=E741",
          # do not assign a lambda expression, use a def
          "--ignore=E731",
          # found useless expression. ignore since .qmd displays
          "--ignore=B018",
          # {name} is too complex ({complexity} > {max_complexity})
          # note: ignored on select repositories
          "--ignore=C901",
          # E and W: pycodestyle, standard PEP8 errors and pycodestyle warnings.
          # F: pyflakes warnings (e.g., unused variables, undefined names).,
          # B: flake8-bugbear (useful best practices).
          # SIM: flake8-simplify
          # C90: McCabe complexity (cyclomatic complexity).
          # UP: pyupgrade, Python version compatibility
          "--select=E,W,F,B,C90,UP,SIM",
          # linter checks for lines, but doesn't fix, default is 88
          "--line-length=79",
          # lint all files in the current directory, and fix any fixable errors.
          "--fix"]
    # run the ruff-formatter; the Ruff formatter is an extremely fast
    # Python code formatter designed as a drop-in replacement for Black
    -   id: ruff-format
        args: [
          "--line-length=79",
        ]

[SamuelBrand1]

• Beyond the ruff hook, I do not see why the pre-commit file needs to be simplified, generally speaking. The additional hooks removed have been helpful at times albeit, for some of them, quite infrequently.

I was going with the pyrenew-hew pre-commit config plus some extras that were here. Maybe, isort to go back in. The purpose of the PR was to get rid of Quarto checking as a hook.

[SamuelBrand1]

• I still need to check on my end if the numpyro upgrade causes CI errors.

It is passing the pytest CI (see this PR #65 ). Are there any other checks?

Its also separate to the pre-commit decision somewhat, although I noticed a problem in variation between the README created by pre-commit locally for me and on the GH runner causing pre-commit action to fail.

[SamuelBrand1]

Just noticed you had more hooks than in pyrenew-hew I've added them back in.

[SamuelBrand1]

From f2f it sounds a bit like I'm asking to unwind a recent change... which is obviously a bit painful.

The core issue here is that it seems that different local systems render the README just a little bit differently. Since this is a fail mode for the pre-commit for the CI, that either seems a bit brittle or limits local dev. What do you think @dylanhmorris ?

[dylanhmorris]

The core issue here is that it seems that different local systems render the README just a little bit differently. Since this is a fail mode for the pre-commit for the CI, that either seems a bit brittle or limits local dev. What do you think @dylanhmorris ?

I agree. I had also been having trouble with the README in local pre-commit, and had been meaning to open an issue. Like you, I am somewhat skeptical of dynamically rendered Github README.md files (versus docs/websites):

My view is that render operations relying on the code should live in documentation rather than the README but YMMV here I guess.

If @AFg6K7h4fhy2 is committed to a dynamic README, though, I think our experiences here point toward local pre-commit not being the best place to do it (versus CI).

[AFg6K7h4fhy2]

Re: #67 (comment)

I am not committed to dynamically updated README, and the original script I had was motivated by Damon's comments.

I am interested in doing whatever minimizes the combinations of user-problems and manual labor.

[AFg6K7h4fhy2]

Some comments & changes:

• Reverted back to desired .pre-commit-config.yaml.
• Added security hook detect-secrets.
• Changed files to adhere to pre-commit (all in forecasttools)
• Modified README.qmd w/ minor change to trigger re-render.
  • Noticed that README.md did not have change.
  • Noticed that README action ran successful.
    • Doesn't seem to have modified README.md in PR.
• If I am missing something, please let me know.
  • If README.md can be rendered, I will approve this PR.

[dylanhmorris]

Some comments & changes:

• Reverted back to desired .pre-commit-config.yaml.

• Added security hook detect-secrets.

• Changed files to adhere to pre-commit (all in forecasttools)

• Modified README.qmd w/ minor change to trigger re-render.

  • Noticed that README.md did not have change.

  • Noticed that README action ran successful.

    • Doesn't seem to have modified README.md in PR.

• If I am missing something, please let me know.

  • If README.md can be rendered, I will approve this PR.

It looks to me like the render readme CI action does not commit the result. If you want the readme rendered in CI to become part of the PR, it needs to be staged and committed. I still think a dynamically rendered readme.md for the repo may not be the best use of time.

[SamuelBrand1]

Some comments & changes:

• Reverted back to desired .pre-commit-config.yaml.

• Added security hook detect-secrets.

• Changed files to adhere to pre-commit (all in forecasttools)

• Modified README.qmd w/ minor change to trigger re-render.

  • Noticed that README.md did not have change.

  • Noticed that README action ran successful.

    • Doesn't seem to have modified README.md in PR.

• If I am missing something, please let me know.

  • If README.md can be rendered, I will approve this PR.

It looks to me like the render readme CI action does not commit the result. If you want the readme rendered in CI to become part of the PR, it needs to be staged and committed. I still think a dynamically rendered readme.md for the repo may not be the best use of time.

Yes, I listed that as the main downside. The new CI just checks that it runs (which is IMO handy). I also don't want to fiddle with staging the change!

[SamuelBrand1]

This PR is a bit me and a bit @AFg6K7h4fhy2 now! So @dylanhmorris gets the reviewer vote.

[dylanhmorris]

I think the approach here is good for now and long run we should move away from a dynamic readme.