-
Notifications
You must be signed in to change notification settings - Fork 15
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
base: main
Are you sure you want to change the base?
Conversation
5bc8019
to
0063d5b
Compare
0063d5b
to
7c9d715
Compare
…tants (fortitude P001).
…nting workflow file.
There was a problem hiding this 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!
Will review properly during the hackathon today. |
There was a problem hiding this 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).
|
||
- name: fortitude source | ||
if: always() | ||
run: | | ||
cd ${{ github.workspace }} | ||
. ftorch/bin/activate | ||
fortitude check src/ | ||
|
||
- name: clang source |
There was a problem hiding this comment.
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.
style: 'file' # Use .clang-format config file | ||
tidy-checks: '' # Use .clang-tidy config file |
There was a problem hiding this comment.
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?
torchscript_file_error = ( | ||
f"Saved TorchScript file {os.path.join(filepath, saved_ts_filename)} " | ||
"cannot be found." | ||
) | ||
raise FileNotFoundError(model_error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
torchscript_file_error = ( | ||
f"Saved TorchScript file {os.path.join(filepath, saved_ts_filename)} " | ||
"cannot be found." | ||
) | ||
raise FileNotFoundError(model_error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
torchscript_file_error = ( | ||
f"Saved TorchScript file {os.path.join(filepath, saved_ts_filename)} " | ||
"cannot be found." | ||
) | ||
raise FileNotFoundError(model_error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f" calculated gradient b.grad:\n{b.grad}\n" | |
f"calculated gradient b.grad:\n{b.grad}\n" |
torchscript_file_error = ( | ||
f"Saved TorchScript file {os.path.join(filepath, saved_ts_filename)} " | ||
"cannot be found." | ||
) | ||
raise FileNotFoundError(model_error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
* 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/) |
There was a problem hiding this comment.
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?
pip install torch torchvision --index-url https://download.pytorch.org/whl/cpu | ||
pip install fortitude-lint | ||
pip install ruff |
There was a problem hiding this comment.
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.
VN=$(python -c "import sys; print('.'.join(sys.version.split('.')[:2]))") | ||
export Torch_DIR=${VIRTUAL_ENV}/lib/python${VN}/site-packages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is simpler?
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: //") |
VN=$(python -c "import sys; print('.'.join(sys.version.split('.')[:2]))") | ||
export Torch_DIR=${VIRTUAL_ENV}/lib/python${VN}/site-packages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
pip install fortitude-lint | ||
pip install ruff |
There was a problem hiding this comment.
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:
- In case they get updated the CI will start to fail
- 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 }} |
There was a problem hiding this comment.
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 👀
There was a problem hiding this comment.
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...
😅
Add static analysis to the repo using: