-
Notifications
You must be signed in to change notification settings - Fork 9
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
Major Refactor: Add Ruff Linting, Nox, and Pre-Commit Hook #266
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #266 +/- ##
==========================================
- Coverage 88.60% 88.54% -0.07%
==========================================
Files 38 38
Lines 3405 3386 -19
==========================================
- Hits 3017 2998 -19
Misses 388 388 ☔ View full report in Codecov by Sentry. |
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.
Thanks for putting together this (huge) PR Hanna!
- It would be good to run pre-commit as part of the CI too -- when I pulled down the branch and ran it, it made some additional changes (not related to ruff, but the other hooks, which I think we'd also want to run regularly). From poking around, it looks like it's impossible to run something like "pre-commit check", which would check whether the hooks would pass and fail, but not change anything. Therefore, I think the only real CI option is pre-commit.ci, which will also push back to the repo. It's not ideal (I'd like to have as much as possible explicit in the git repo, rather than involving stuff that has to be configured through separate websites), but still fine I think. Does that match your understanding as well?
- The contributing document has gotten complex as part of this. I think it needs a run through to improve the flow, but that's not part of this PR
- Can you add your notes about the wildcard imports and the two approaches to Improve API #246?
- it looks like the right way to handle F401 in init files is with
__all__
, which we should do anyway. I added a note to Improve API #246. - Can you move the next steps of your PR note to a relevant issue? maybe Consistent formatting (Python Black) #51 ?
- you have several comments of the form
or similar. is the ignore line doing anything there? I don't think it adds any info beyond the noqa line (which we do explain in contributing), so if it's not doing anything functional, it should be removed
# ignore F401 # ruff: noqa: F401
- There are several changes that I think were not made by ruff or pre-commit? I flagged them, asking "why this change". I'm not necessarily opposed to them, but want to double-check whether they were made directly by you or by ruff and, if by you, want to understand your thought process
device = torch.device("cpu") | ||
else: | ||
device = std.device | ||
device = torch.device("cpu") if isinstance(std, float) else std.device |
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 preferred?
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 if-else block is flagged by SIM108 (from the flake8-simplify linter). Ternary operators are preferred as it is more concise.
@@ -339,10 +360,15 @@ def __init__( | |||
): | |||
super().__init__() | |||
if pretrained: | |||
assert kernel_size in [31, (31, 31)], "pretrained model has kernel_size (31, 31)" | |||
assert kernel_size in [ |
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 ugly. I should probably just rewrite it to use if so it formats nicer...
@@ -331,7 +366,7 @@ def _check_convergence(self, stop_criterion, stop_iters_to_check): | |||
Have we been synthesizing for ``stop_iters_to_check`` iterations? | |||
| | | |||
no yes | |||
| '---->Is ``abs(synth.loss[-1] - synth.losses[-stop_iters_to_check]) < stop_criterion``? | |||
| '---->Is abs(synth.loss[-1] - synth.loss[-stop_iters_to_check]) < stop_crit? |
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 this change?
@@ -274,7 +298,7 @@ def _check_convergence(self, stop_criterion, stop_iters_to_check): | |||
Have we been synthesizing for ``stop_iters_to_check`` iterations? | |||
| | | |||
no yes | |||
| '---->Is ``abs(synth.loss[-1] - synth.losses[-stop_iters_to_check]) < stop_criterion``? | |||
| '----> Is the change in loss < stop_criterion over ``stop_iters_to_check``? |
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 this change?
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.
Those changes were made to honor the line-length limit. Alternatively, we can ignore too long lines here with #noqa: E501.
| '---->Is the change in loss < stop_criterion over ``stop_iters_to_check``? | ||
| no | | ||
| | yes | ||
|-------' '---->Have we synthesized all scales and done so for ``ctf_iters_to_check`` iterations? | ||
|-------' '---->Are all scales synthesized for `ctf_iters_to_check` iterations? |
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 this change?
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.
Those changes were made to honor the line-length limit. Alternatively, we can ignore too long lines here with #noqa: E501.
… into code-formatting merge.
… but doesn't belong into this PR
Co-authored-by: William F. Broderick <[email protected]>
Co-authored-by: William F. Broderick <[email protected]>
Co-authored-by: William F. Broderick <[email protected]>
Co-authored-by: William F. Broderick <[email protected]>
Co-authored-by: William F. Broderick <[email protected]>
Co-authored-by: William F. Broderick <[email protected]>
Co-authored-by: William F. Broderick <[email protected]>
Co-authored-by: William F. Broderick <[email protected]>
Co-authored-by: William F. Broderick <[email protected]>
…eplacing the None, None expression, for better readability
… using this linter
Co-authored-by: William F. Broderick <[email protected]>
Co-authored-by: William F. Broderick <[email protected]>
Summary:
This PR refactors the project by adding Ruff Linting and formatting, integrated with Nox and an optional Pre-commit hook. It enhances both code quality and development efficiency.
Key Changes:
Next Steps & Open Questions:
Resolves issue #51
Configuration Details:
1. Ruff Linting Configuration:
The current Ruff configuration includes the most commonly used linting rules. (Future PRs will likely incorporate more linting rules, as discussed here).
E
): Enforces style consistency based on PEP8.F
): Detects basic errors such as undefined variables or missing imports.UP
): Ensures usage of modern Python syntax, making our code more future-proof.SIM
): Improves code readability by suggesting simplified expressions. We have chosen to ignore ruleSIM105
since our team prefers explicit boolean comparisons in some cases.I
): Ensures imports are organized and ordered consistently, improving code readability and maintainability.Note that a couple of linting rules were ignored with
#noqa <ruff rule>
(ignoring a single line) and#ruff: noqa <ruff rule>
(ignoring entire file):E501
for too long lines has been ignored several times to ensure for example that links will still work or flow charts remain readableI001
sorting imports has been ignored once insrc/plenoptic/__init__.py
as the import order matters to avoid circular dependencies and tests to failF401
unused imports# ruff: noqa: F401
is used in several__init__.py
files to ignore unused imports. When unused imports are removed, tests fail.SIM105
: Checks fortry
-except
-pass
blocks that can be replaced with thecontextlib.suppress
context manager.pyproject.toml
F403
flags wildcard imports*
, which is bad practice.To remove wildcard (
*
) imports, issue Improve API #246 should be considered. 2 recommended approaches are:__init__.py
File:from .validate import remove_grad
plenoptic.tools.validate
from . import validate
within the subpackage.validate.py
, define a list called__all__ = ["remove_grad", ...]
, which specifies what can be imported from the file.__init__.py
, and it controls the public API of that module.2. Ruff Formatting
Currently, formatting is enforced to emulate Black in
pyproject.toml
:Nox & Pre-commit Hooks and their Configurations:
TL;DR:
noxfile.py
.pip install pre-commit
. Pre-commit checks can be ignored, by adding--no-verify
to your commit message.2. Nox Configuration:
Nox is a Python-based task runner that is used to automate various development workflows, such as testing, building, and deploying applications. It allows you to define sessions, which are Python functions in the
noxfile.py
decorated with@nox.session
with various parameters, such asname
, by which these sessions can then be executed via the command line. This easily allows to test code by running pre-defined tasks with a single command across multiple Python versions without manually setting up environments.Example: Based on the following
noxfile.py
, the commandnox -s tests
would run the session named "tests” below.nox
without any commands would run all the sessions defined in thenoxfile.py
.Here are the current Nox sessions available for execution:
nox -s lint
nox -s tests
nox -s coverage
3. Pre-Commit Configurations:
Hook:
A pre-commit hook can be added to run Ruff linting on code before it is committed. This ensures that developers catch and fix linting issues early in the development process. It only runs when installed and can be bypassed with
git commit -m <my commit message> --no-verify
.Considered Configurations: (in
.pre-commit.yaml
:(Legend: ✅: included, ❓: recommended, but not included, possibly in future PR)
check-added-large-files
✅check-case-conflict
✅File.txt
andfile.txt
would be considered the samecheck-merge-conflict
✅<<<<<<<
,=======
,>>>>>>>
) in files.check-symlinks
❓check-yaml
✅debug-statements
✅print
,console.log
) left in code.end-of-file-fixer
✅trailing-whitespace
✅CI Pipeline:
Finally, the CI configuration
ci.yml
has been updated to include a step each for running Ruff formatting and linting check on all new commits and pull requests. Two separate actions were chosen to signal the developer which combination of the two might be causing an error.Next steps:
0. update contributing file
1.Ruff Linting
Include the following linting rules (in
pyproject.toml
) and resolve linting errors:flake8-bugbear
):Checks for common programming errors, style issues, or patterns that are error-prone
flake8-unused-arguments
):Flags unused function arguments.
flake8-comprehensions
):Checks for issues in list comprehensions and generator expressions.
flake8-errmsg
):Lints error message formatting.
flake8-import-conventions
):Ensures imports are formatted according to best practices.
flake8-use-pathlib
):Encourages the use of
pathlib
instead ofos.path
.flake8-return
):Checks for issues related to return statements.
flake8-simplify
):Suggests simplifications for code.
flake8-print
):Flags print statements.
pyupgrade
):Upgrades syntax to modern Python standards.
flake8-2020
):Enforces Python 3.8+ compatibility.
2. Nox:
We might want to consider to include nox sessions for:
mypy
or a similar tool to perform static type checking, to catch type-related errors early.3. Pre-commit
We might want to add the following to
.pre-commit-config.yaml
:mixed-line-ending
requirements-txt-fixer
requirements.txt
files, ensuring they follow conventions and are easy to manage.