Initial ruff config and azure setup

[eirikurj]

Purpose

This PR holds the initial ruff.toml config and Azure setup.

Feel free to commit and update as needed.

Addresses #73

Open PRs testing this config

• Test ruff formatting and linting adflow#392
• Test ruff linting and formatting pygeo#275
• Test using pre-commit/ruff GHA from .github repo baseclasses#100
• Test ruff formatting and linting pyoptsparse#454

Expected time until merged

Type of change

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (non-backwards-compatible fix or feature)
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Documentation update
• Maintenance update
• Other (please describe)

Testing

Checklist

• I have run flake8 and black to make sure the Python code adheres to PEP-8 and is consistently formatted
• I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
• I have run unit and regression tests which pass locally with my changes
• I have added new tests that prove my fix is effective or that my feature works
• I have added necessary documentation

[ewu63]

I don't know if there's appetite to move these "quick" CI checks to GHA but that may offer easier maintainability + slightly faster spoolup. Then we could also remove them from Azure so we aren't maintaining both CI workflows.

[A-CGray]

I don't know if there's appetite to move these "quick" CI checks to GHA but that may offer easier maintainability + slightly faster spoolup. Then we could also remove them from Azure so we aren't maintaining both CI workflows.

Oh yeah for sure, we can skip the overhead of the docker image setup and the avoid taking up the limited runners we have for private repos.

Is there actually a way we can define the actions in this repo and then use them in our other repos though?

[A-CGray]

Is there actually a way we can define the actions in this repo and then use them in our other repos though?

Looks like the answer is sort of, we can define a reusable workflow in this repo, but we'll still need to create a workflow in all our individual repos that calls the one defined here.

nvm, I am very dumb and didn't realise we already have a bunch of GHA workflows setup exactly like this

[A-CGray]

See mdolab/baseclasses#100 for an example of a working GHA check using pre-commit

[A-CGray]

@eirikurj @ewu63 is it necessary for us to repeat the check with two different target versions? I had just copied that from our flake8 workflow, but since --target-version specifies the minimum python version the code should support, won't checking with py39 automatically mean the checks will pass for py310 and above?

[A-CGray]

It would be great if we could get this merged by the end of this week. We're having the lab workshop next week and I was hoping to introduce the new students to our development process by having them open PRs on our repos to update their formatting.

[ewu63]

@eirikurj @ewu63 is it necessary for us to repeat the check with two different target versions? I had just copied that from our flake8 workflow, but since --target-version specifies the minimum python version the code should support, won't checking with py39 automatically mean the checks will pass for py310 and above?

That logic makes sense to me.

[ewu63]

The impl looks okay, I'll defer to others for hashing out the exact set of checks to run with ruff. What is the plan with GHA/Azure? Do you plan to deprecate the Azure jobs for flake8/black/isort? I imagine you'd still want to keep pylint right? Since it in theory covers separate cases.

[A-CGray]

The impl looks okay, I'll defer to others for hashing out the exact set of checks to run with ruff. What is the plan with GHA/Azure? Do you plan to deprecate the Azure jobs for flake8/black/isort? I imagine you'd still want to keep pylint right? Since it in theory covers separate cases.

I would vote to just remove the Azure flake8 and black checks. I think we were planning on keeping isort separate since not all our repos use it, and with ruff you need to edit the config to enable/disable it. I'm happy to keep our current pylint setup, which repos actually use it though?

My suggestion for next steps would be:

1. Do a bit more work on the config to try and get parity with our current flake8 config
2. Remove Azure black and flake8 stuff from the repo in this PR, keep isort and pylint as is.
3. Merge this PR
4. Open and merge PRs on our repos during the workshop to add the new GHA workflow and fix any new formatting linting errors

[ewu63]

Hmm any reason we're not running isort on all repos? I can't really think of a reason not to, esp if we are enforcing black everywhere.

[eirikurj]

is it necessary for us to repeat the check with two different target versions?

From what I can tell from the docs, it is not necessary. You specify the minimum version you want to support (currently in our case 3.9) and it should run on everything above it. Using newer python syntax will get flagged.

Hmm any reason we're not running isort on all repos? I can't really think of a reason not to, esp if we are enforcing black everywhere.

I agree that we should run this by default. However, currently this option is by default false so you do need to specify ISORT: true in azure-pipelines.yaml. I think currently only a handful of repos actually do this. Don't recall the original motivation at the moment, but we should probably change this and default to true and instead just opt-out on a per-repo basis. This might not be needed if we just enforce isort as well as part of this PR.

We should work more on the ruff.toml rules and I do think we need to test out and adjust further before merging, especially if we want to obtain get the same behavior as with current black/flake8. For example, there are missing ignores compared to the current config flake8 config. However, I think this might also be a good time to remove as many of the ignores as possible and just adjust/fix the code, so its maybe not worth spending time on getting an exact match. In any case we need to do some testing on our repos.

Regarding next steps or a plan, I think the proposed plan is fine except I would prefer to have an active and non-failing linting/formatting checks in place. Otherwise, we will have failing ruff checks on other PRs while we work on the MDO Lab codes. So we should not remove black/flake8 and merge this PR unless the MDO Lab codes have ruff modification PRs ready to be merged.

Another option is (this may be overly cautious) is to run ruff in a transition phase in parallel to our current tests, but allow it to fail so its not artificially blocking any PRs. This allows us to merge this PR. Independently, we can work on updating MDO Lab repos with ruff formatting. With this approach, there is no blocking, and we can deal with this incrementally and adjust the config as needed. Once ready, we can remove black/flake8 (and isort if applicable) in a separate PR.

[A-CGray]

Need to implement ability to combine the .github ruff config with any local ruff.toml, like we currently do for flake8 conflict

[kanekosh]

Need to implement ability to combine the .github ruff config with any local ruff.toml, like we currently do for flake8 conflict

Haven't tried but this can be used? https://docs.astral.sh/ruff/settings/#extend

This would probably require people to have .github/ruff.toml in the exact path specified in local ruff.tomls which might not be ideal

[A-CGray]

Need to implement ability to combine the .github ruff config with any local ruff.toml, like we currently do for flake8 conflict

Haven't tried but this can be used? https://docs.astral.sh/ruff/settings/#extend

This would probably require people to have .github/ruff.toml in the exact path specified in local ruff.tomls which might not be ideal

Good call, I ended up using that option here, kind of similar to what this repo currently does to handle repo-specific flake8 configuration files.

[kanekosh]

Thanks @A-CGray abnd @eirikurj . I'm wondering why we run ruff checks both in Azure and GHA via pre-commit, could we just do one of these? Are we running pre-commit GHA for testing only, and once we merge this PR we remove pre-commit GHA yaml and only use Azure checks?

[kanekosh]

commenting so we don't forget this

[A-CGray]

Done

[eirikurj]

I looked at the output for the PRs that are up (added a link here also in the description above). I think we are getting close with the config, but I added a few comments.

[eirikurj]

Related to the above comment, for users that are developing locally and want to run the same thing, its seems like they would need to manually add extend = <path-to-global-ruff.toml> to all project files? We could add/have a script that does this automatically that could be used both here and locally.

[A-CGray]

Yeah, it's annoying, but currently they'd have to do the same thing for our repos with local flake8 configs so I don't think this is any worse.

[eirikurj]

Its not quite as difficult with flake8 since on the CLI you can just add the --append-config flag with the extra config file. No file editing needed. This does not seem to exist in ruff.

[eirikurj]

Is the idea that we check all files with all these? This seems to be going through everything, including fortran files and other source and config files, generating loads of errors. Maybe this is fine since these are mostly whitespace issues, but ideally we should have tools specifically for fortran to avoid any clashes.

[A-CGray]

Yeah I think we should be running this on all files. I often find myself unwittingly commiting a bunch of whitespace changes to files I didn't mean to edit because my editor automatically trims whitespace and fixes the ends of files. Enforcing these checks will avoid that.

[eirikurj]

Fixing is useful for local development, but as part of CI its not since we do want the issues to be reported.

[A-CGray]

I believe that pre-commit flags a check as failed if any files are modified, even if the check returns a zero exit code. So I believe this would work as is, even if all the issues ruff check finds are fixable. Just to be sure, I added the --exit-non-zero-on-fix flag to make sure we get a non-zero exit code even if all the issues found are fixable.

[eirikurj]

Here I was not thinking about the exit code. We should not have it auto-fix since we want to see what linting codes are triggered. The dig diff output at the end is not sufficient for this.

[A-CGray]

Thanks @A-CGray abnd @eirikurj . I'm wondering why we run ruff checks both in Azure and GHA via pre-commit, could we just do one of these? Are we running pre-commit GHA for testing only, and once we merge this PR we remove pre-commit GHA yaml and only use Azure checks?

I would vote we remove the ruff Azure checks altogether and run them via GHA for all repos. I'd also like to move our other formatting/style checks (basically anything quick that doesn't require spinning up our docker images) from Azure to GHA as well. But we should save that for another PR.

I have removed the Azure ruff check, but you can git revert c0c2e18 if you'd like to keep it.

[eirikurj]

Overall, I think I am satisfied with the ruff.toml config, at least based on what I have seen on the repos.
However, just want to clarify, the formatting checks are not pulling a full mdolab docker image, its only pulling a minimal ubuntu vm (see here), which should be the same as on GHA. Its should only be the repository tests that pull the mdolab images.

On Azure or GHA I am favoring Azure at the moment due to a couple of reasons.

• We're currently paying for Azure runners. This is important since I dont think GHA actions runners are free for private repositories. This means that if we hit the quota we would need to buy more credits for private repositories.
• We have a pretty nice uniform environment and framework that we have set up, with shared config files. With this setup, there are no extra config files to maintain, only the azure yaml file (and flags to control behavior). This makes maintenance simpler.

Are there any benefits moving from Azure to GHA that I am missing? I dont mind moving to GHA, I just want to make sure we are not limiting us in any way. Let me know what you think. Curious to hear from others as well @kanekosh @ewu63.

[A-CGray]

Overall, I think I am satisfied with the ruff.toml config, at least based on what I have seen on the repos. However, just want to clarify, the formatting checks are not pulling a full mdolab docker image, its only pulling a minimal ubuntu vm (see here), which should be the same as on GHA. Its should only be the repository tests that pull the mdolab images.

On Azure or GHA I am favoring Azure at the moment due to a couple of reasons.

* We're currently paying for Azure runners. This is important since I dont think GHA actions runners are free for private repositories. This means that if we hit the quota we would need to buy more credits for private repositories.

* We have a pretty nice uniform environment and framework that we have set up, with shared config files. With this setup, there are no extra config files to maintain, only the azure yaml file (and flags to control behavior). This makes maintenance simpler.

Are there any benefits moving from Azure to GHA that I am missing? I dont mind moving to GHA, I just want to make sure we are not limiting us in any way. Let me know what you think. Curious to hear from others as well @kanekosh @ewu63.

These are good points, I forgot about the limits for GHA on private repos. I think since we are technically under the UM enterprise GitHub our limit is something like 50,000 minutes per month, so we'd never come anywhere near it with just these formatting checks, but good to not be beholden to the whims of GitHub anyway. I'll add back the Azure config and then I'll go ahead and merge this so that our repos starting running the ruff checks on Azure.

[kanekosh]

I'm ok with either tools.

One thing I like about GHA is that it is easier (less clicks) to see what failed from a Github PR tab.
GHA has the minute limits on private repos (see here), but we have more than enough as we're under UM Enterprise Cloud I believe (even on a free account the limit is 2000min/mo, which should be enough for linting and formatting).

[ewu63]

Some quick thoughts here

• I personally prefer using GHA for "simple" tasks such as linting, since the VMs are spooled up faster (or maybe this is just how it feels) and it offers better integration with status checks etc. Not a huge deal but supporting both runners does take resources. The original intention I had when I set this up was to slowly move away from Azure since GHA added templated jobs soon after, and then we can eventually stop paying for Azure runners (i.e. move to self-hosted runners for private repos). Again, not a big deal but the lab should have a long-term plan for Azure.
• Which repos need some special override? I haven't tried this but ruff claims to expand home directory and env vars, so it may be sufficient to just have extend = ~/.config/ruff/ruff.toml in any repo that has an override, then the location of the default config file is standardized between users and also on GHA/Azure (if wget also puts it in that location).