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

Add static analysis using fortitude, clang, and ruff #187

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

jatkinson1000
Copy link
Member

@jatkinson1000 jatkinson1000 commented Nov 11, 2024

Add static analysis to the repo using:

  • fortitude for Fortran
  • clang format and tidy for C and C++
  • ruff for Python
  • Apply workflows for these checks
  • Update developer docs etc to note these

@jatkinson1000 jatkinson1000 force-pushed the static-analysis branch 14 times, most recently from 5bc8019 to 0063d5b Compare November 11, 2024 11:39
Copy link
Contributor

@jwallwork23 jwallwork23 left a comment

Choose a reason for hiding this comment

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

Looks great! Happy for this to be submitted once a few small things are addressed.

The software citations for (Ruff, Fortitude, Clang-*, TorchFort, CAM-GW, ...) show as 'n.d' (no date) in the pdf. Does the 'year' attribute need to be set as well as the 'accessed' attribute? The date of access doesn't show in the references anyway.

Argh wrong PR!

@jwallwork23
Copy link
Contributor

Argh wrong PR!

Will review properly during the hackathon today.

Copy link
Contributor

@jwallwork23 jwallwork23 left a comment

Choose a reason for hiding this comment

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

This is a great contribution, thanks @jatkinson1000! A few comments and requested changes.

Worth noting that for nextSIM-DG we ended up pinning clang-format version 18 because the default settings sometimes change between versions and we hadn't set any of them in the config file (nor have you here, it seems).

Comment on lines +54 to +62

- name: fortitude source
if: always()
run: |
cd ${{ github.workspace }}
. ftorch/bin/activate
fortitude check src/

- name: clang source
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps simple comments like # Apply Fortran linter etc. for clarity.

Comment on lines +69 to +70
style: 'file' # Use .clang-format config file
tidy-checks: '' # Use .clang-tidy config file
Copy link
Contributor

Choose a reason for hiding this comment

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

These config files don't seem to exist on the branch. Should I expect them to?

Comment on lines +175 to +179
torchscript_file_error = (
f"Saved TorchScript file {os.path.join(filepath, saved_ts_filename)} "
"cannot be found."
)
raise FileNotFoundError(model_error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
torchscript_file_error = (
f"Saved TorchScript file {os.path.join(filepath, saved_ts_filename)} "
"cannot be found."
)
raise FileNotFoundError(model_error)
torchscript_file_error = (
f"Saved TorchScript file {os.path.join(filepath, saved_ts_filename)} "
"cannot be found."
)
raise FileNotFoundError(torchscript_file_error)

Comment on lines +181 to +185
torchscript_file_error = (
f"Saved TorchScript file {os.path.join(filepath, saved_ts_filename)} "
"cannot be found."
)
raise FileNotFoundError(model_error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
torchscript_file_error = (
f"Saved TorchScript file {os.path.join(filepath, saved_ts_filename)} "
"cannot be found."
)
raise FileNotFoundError(model_error)
torchscript_file_error = (
f"Saved TorchScript file {os.path.join(filepath, saved_ts_filename)} "
"cannot be found."
)
raise FileNotFoundError(torchscript_file_error)

examples/2_ResNet18/resnet_infer_python.py Show resolved Hide resolved
Comment on lines +168 to +172
torchscript_file_error = (
f"Saved TorchScript file {os.path.join(filepath, saved_ts_filename)} "
"cannot be found."
)
raise FileNotFoundError(model_error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
torchscript_file_error = (
f"Saved TorchScript file {os.path.join(filepath, saved_ts_filename)} "
"cannot be found."
)
raise FileNotFoundError(model_error)
torchscript_file_error = (
f"Saved TorchScript file {os.path.join(filepath, saved_ts_filename)} "
"cannot be found."
)
raise FileNotFoundError(torchscript_file_error)

assert torch.allclose(-2 * b, b.grad)
if not torch.allclose(9 * a**2, a.grad):
result_error = (
f" calculated gradient a.grad:\n{a.grad}\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
f" calculated gradient a.grad:\n{a.grad}\n"
f"calculated gradient a.grad:\n{a.grad}\n"

raise ValueError(result_error)
if not torch.allclose(-2 * b, b.grad):
result_error = (
f" calculated gradient b.grad:\n{b.grad}\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
f" calculated gradient b.grad:\n{b.grad}\n"
f"calculated gradient b.grad:\n{b.grad}\n"

Comment on lines +177 to +181
torchscript_file_error = (
f"Saved TorchScript file {os.path.join(filepath, saved_ts_filename)} "
"cannot be found."
)
raise FileNotFoundError(model_error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
torchscript_file_error = (
f"Saved TorchScript file {os.path.join(filepath, saved_ts_filename)} "
"cannot be found."
)
raise FileNotFoundError(model_error)
torchscript_file_error = (
f"Saved TorchScript file {os.path.join(filepath, saved_ts_filename)} "
"cannot be found."
)
raise FileNotFoundError(torchscript_file_error)

Comment on lines +106 to +107
* C++: [clang-format](https://clang.llvm.org/docs/ClangFormat.html) and [clang-tidy](https://clang.llvm.org/extra/clang-tidy/)
* C: [clang-format](https://clang.llvm.org/docs/ClangFormat.html) and [clang-tidy](https://clang.llvm.org/extra/clang-tidy/)
Copy link
Contributor

Choose a reason for hiding this comment

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

I gather that clang-tidy is for linting and clang-format is for formatting? Perhaps worth mentioning here?

Comment on lines +40 to +42
pip install torch torchvision --index-url https://download.pytorch.org/whl/cpu
pip install fortitude-lint
pip install ruff
Copy link
Member

Choose a reason for hiding this comment

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

I think it's worth making this a requirements.txt. It will make it easier to reproduce failure locally.

Or, we should at least fix the versions of ruff/fortitude. E.g., in case ruff add/remove linting rules in a newer version.

Comment on lines +48 to +49
VN=$(python -c "import sys; print('.'.join(sys.version.split('.')[:2]))")
export Torch_DIR=${VIRTUAL_ENV}/lib/python${VN}/site-packages
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this is simpler?

Suggested change
VN=$(python -c "import sys; print('.'.join(sys.version.split('.')[:2]))")
export Torch_DIR=${VIRTUAL_ENV}/lib/python${VN}/site-packages
export Torch_DIR=$(pip show torch | grep Location | sed "s/Location: //")

Comment on lines +48 to +49
VN=$(python -c "import sys; print('.'.join(sys.version.split('.')[:2]))")
export Torch_DIR=${VIRTUAL_ENV}/lib/python${VN}/site-packages
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
VN=$(python -c "import sys; print('.'.join(sys.version.split('.')[:2]))")
export Torch_DIR=${VIRTUAL_ENV}/lib/python${VN}/site-packages
export Torch_DIR=$(pip show torch | grep Location | sed "s/Location: //")

one liner for you :) little simpler IMO

Comment on lines +41 to +42
pip install fortitude-lint
pip install ruff
Copy link
Member

Choose a reason for hiding this comment

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

we should make a requirements.txt for this:

  1. In case they get updated the CI will start to fail
  2. Make it easier to reproduce locally and use the same version

uses: cpp-linter/cpp-linter-action@v2
id: linter
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
Copy link
Member

Choose a reason for hiding this comment

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

why does it need a token? just curious 👀

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, because the action said to use this format...
😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants