C++ and Python Linting + Formatting + Format checks, including pre-commit and pre-push for GPU-code unit testing + CI #5
Conversation
MET-7 Pre-commit
Deliverable:
MET-12 pre-commit hook for linting and formatting in both C++ and Python |
|
Part of MET-6 In this PR, I also add in the CI support. It is really simple. All it does is create the virtual environment on a github runner, installs pixi, installs the For now, I don't think it is worth the effort to set up Spot GPU runners to do remote GPU CI actions. The pre-push should be good enough fo rnow. |
arijit-dasgupta
changed the title
mugamma
left a comment
mugamma
left a comment
There was a problem hiding this comment.
Great job dude! My overall comment is let's slim this down significantly. I don't think we need 7 scripts for linting. Let's just have a single simple lint/format/fix pixi taks, which is also called in the hooks. It's fine if this task is calling a script. I love the dev-setup task, and I think it should just do all that is needed for the setup of the dev env.
So at the end of the day it would be good to have
- A script for setting up the repo (also runnable through
pixi dev-setup) - A script for linting and formatting (also runnable through
pixi lint) - Hooks that call the second script
| - name: Check formatting | ||
| run: pixi run format-check | ||
|
|
||
| - name: Lint code | ||
| run: pixi run lint | ||
|
|
There was a problem hiding this comment.
Let's also add the tests here. It's actually more important to run the tests than it is to do formatting and linting.
There was a problem hiding this comment.
The issue here is that the ci has no GPU, so no CUDA code will run. That's why it can be more of a pre-push thing
There was a problem hiding this comment.
I see this makes sense. Let me look into this.
| # Run tests (pre-push hook) | ||
| - id: test | ||
| name: Run all tests | ||
| entry: scripts/test-quiet.sh |
There was a problem hiding this comment.
Why is this not using the pixi run test task that we already have?
There was a problem hiding this comment.
It does do that! (running ctest and pytest), but suppress the output to be quiet. I think everytime we push, it might get messy to have a a barrage of stdout or stderr. But honestly, who cares, we can remove this too.
There was a problem hiding this comment.
Makes sense. TBH I prefer a pixi run test to a whole separate file in the codebase, but I do understand it's a matter of taste. I'm fine with keeping it this way.
README.md
Outdated
| Lint all files: | ||
| ```bash | ||
| pixi run lint | ||
| ``` | ||
|
|
||
| Auto-fix linting issues: | ||
| ```bash | ||
| pixi run lint-fix | ||
| ``` | ||
|
|
||
| **Language-specific commands:** | ||
| - `format-cpp`, `format-python` - Format specific language | ||
| - `format-check-cpp`, `format-check-python` - Check formatting for specific language | ||
| - `lint-cpp`, `lint-python` - Lint specific language | ||
| - `lint-cpp-fix`, `lint-python-fix` - Auto-fix linting issues for specific language | ||
| - `lint-full` - C++/CUDA linting with compile_commands.json (more accurate) | ||
| - `lint-full-fix` - Auto-fix C++/CUDA issues using compile_commands.json |
There was a problem hiding this comment.
Let's just have a single lint command that lints and tries to fix all files.
There was a problem hiding this comment.
sounds good!
pyproject.toml
Outdated
| compile-commands = "cmake -B build -DCMAKE_EXPORT_COMPILE_COMMANDS=ON" | ||
| # Combined formatting and linting (C++/CUDA + Python) | ||
| format = { cmd = "scripts/format/all.sh" } | ||
| format-check = { cmd = "scripts/format/check-all.sh" } | ||
| lint = { cmd = "scripts/lint/all.sh" } | ||
| lint-fix = { cmd = "scripts/lint/fix-all.sh" } | ||
| # C++/CUDA only | ||
| format-cpp = { cmd = "scripts/format/cpp.sh" } | ||
| format-check-cpp = { cmd = "scripts/format/cpp.sh check" } | ||
| lint-cpp = { cmd = "scripts/lint/cpp.sh" } | ||
| lint-cpp-fix = { cmd = "scripts/lint/cpp-fix.sh" } | ||
| # lint-full: Uses compile_commands.json for more accurate C++/CUDA linting | ||
| # (includes proper include paths, defines, and compiler flags from CMake) | ||
| lint-full = { cmd = "scripts/lint/cpp.sh full" } | ||
| lint-full-fix = { cmd = "scripts/lint/cpp-fix.sh full" } | ||
| # Python only | ||
| format-python = { cmd = "scripts/format/python.sh" } | ||
| format-check-python = { cmd = "scripts/format/python.sh check" } | ||
| lint-python = { cmd = "scripts/lint/python.sh" } | ||
| lint-python-fix = { cmd = "scripts/lint/python-fix.sh" } | ||
| # Git hooks (using pre-commit framework) | ||
| install-hooks = { cmd = "pre-commit install && pre-commit install --hook-type pre-push" } | ||
| # Dev setup: install git hooks (for development workflow) | ||
| dev-setup = { cmd = "pre-commit install && pre-commit install --hook-type pre-push" } | ||
| # Pre-commit tasks | ||
| pre-commit-run = { cmd = "pre-commit run --all-files" } | ||
| pre-commit-run-push = { cmd = "scripts/test-quiet.sh" } |
There was a problem hiding this comment.
I think we should definitely simplify this. Let's just have a single simple lint/format/fix command, which is also called in the hooks. I love the dev-setup task, and I think it should just do all that is needed for the setup of the dev env.
| - name: Install dependencies | ||
| run: pixi install |
There was a problem hiding this comment.
The "Install pixi" step (line 18) is already implicitly running pixi install --locked under the hood, so we probably don't need to explicitly run pixi install again. Let's get rid of this in the future :)
Part of MET-7 and part of MET-12.
In this PR, I have set up C++ and Python linting and formatting using
pixi. Python format and linting is done usingruffwhile C++ is done usingclang-tools.We can run them on our own, but they will automatically get triggered via pre-commits hooks anyways. Linting rules could be too strict, so we should feel free to disable warnings that don't make sense.There is also a pre-push hook that runs all the GPU and CPU unit tests (building on top of #2 and #3 ) locally