-
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
runs-on: ubuntu-latest | ||
environment: pypi | ||
permissions: |
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? did this come from one of the linters or elsewhere?
|
||
Your pull request should include information on what you changed and why, referencing any relevant issues or discussions, and highlighting any portion of your changes where you have lingering questions (e.g., "was this the right way to implement this?") or want reviewers to pay special attention. You can look at previous closed pull requests to see what this looks like. | ||
|
||
At this point, we will be notified of the pull request and will read it over. We will try to give an initial response quickly, and then do a longer in-depth review, at which point you will probably need to respond to our comments, making changes as appropriate. We'll then respond again, and proceed in an iterative fashion until everyone is happy with the proposed changes. This process can take a while! (The more focused your pull request, the less time it will take.) | ||
|
||
If your changes are integrated, you will be added as a Github contributor and as one of the authors of the package. Thank you for being part of `plenoptic`! | ||
|
||
### Style guide | ||
### Code Quality and Linting |
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.
### Code Quality and Linting | |
### Code Style and Linting |
|
||
#### Using Ruff | ||
|
||
Ruff is a fast and comprehensive Python formatter and linter that checks for common style and code quality issues. It combines multiple tools, like Pyflakes, pycodestyle, isort, and other linting rules into one efficient tool, which are specified in `pyproject.toml`. Before submitting your code, make sure to run Ruff to catch any issues. |
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.
Ruff is a fast and comprehensive Python formatter and linter that checks for common style and code quality issues. It combines multiple tools, like Pyflakes, pycodestyle, isort, and other linting rules into one efficient tool, which are specified in `pyproject.toml`. Before submitting your code, make sure to run Ruff to catch any issues. | |
Ruff is a fast and comprehensive Python formatter and linter that checks for common style and code quality issues. It combines multiple tools, like black, Pyflakes, pycodestyle, isort, and other linting rules into one efficient tool, which are specified in `pyproject.toml`. Before submitting your code, make sure to run Ruff to catch any issues. See other sections of this document for how to use `nox` and `pre-commit` to simplify this process. |
|
||
Your pull request should include information on what you changed and why, referencing any relevant issues or discussions, and highlighting any portion of your changes where you have lingering questions (e.g., "was this the right way to implement this?") or want reviewers to pay special attention. You can look at previous closed pull requests to see what this looks like. | ||
|
||
At this point, we will be notified of the pull request and will read it over. We will try to give an initial response quickly, and then do a longer in-depth review, at which point you will probably need to respond to our comments, making changes as appropriate. We'll then respond again, and proceed in an iterative fashion until everyone is happy with the proposed changes. This process can take a while! (The more focused your pull request, the less time it will take.) | ||
|
||
If your changes are integrated, you will be added as a Github contributor and as one of the authors of the package. Thank you for being part of `plenoptic`! | ||
|
||
### Style guide | ||
### Code Quality and Linting | ||
We use [Ruff](https://docs.astral.sh/ruff/) for linting and formatting our Python code to maintain a consistent code style and catch potential errors early. To ensure your contributions meet these standards, please follow the guidelines below: |
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 use [Ruff](https://docs.astral.sh/ruff/) for linting and formatting our Python code to maintain a consistent code style and catch potential errors early. To ensure your contributions meet these standards, please follow the guidelines below: | |
We use [Ruff](https://docs.astral.sh/ruff/) for linting and formatting our Python code to maintain a consistent code style and catch potential errors early. We run ruff as part of our CI and non-compliant code will not be merged! |
|
||
Ruff is a fast and comprehensive Python formatter and linter that checks for common style and code quality issues. It combines multiple tools, like Pyflakes, pycodestyle, isort, and other linting rules into one efficient tool, which are specified in `pyproject.toml`. Before submitting your code, make sure to run Ruff to catch any issues. | ||
|
||
**Using Ruff for [Formatting](https://docs.astral.sh/ruff/formatter/#philosophy):** |
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.
**Using Ruff for [Formatting](https://docs.astral.sh/ruff/formatter/#philosophy):** | |
Ruff has two components, a [formatter](https://docs.astral.sh/ruff/formatter/) and a [linter](https://docs.astral.sh/ruff/linter/). Formatters and linters are both static analysis tools, but formatters "quickly check and reformat your code for stylistic consistency without changing the runtime behavior of the code", while linters "detect not just stylistic inconsistency but also potential logical bugs, and often suggest code fixes" (per [GitHub's readme project](https://github.com/readme/guides/formatters-linters-compilers)). There are many choices of formatters and linters in python; ruff aims to combine the features of many of them while being very fast. | |
For both the formatter and the linter, you can run ruff without any additional arguments; our configuration option are stored in the `pyproject.toml` file and so don't need to be specified explicitly. | |
##### Formatting: |
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?
@@ -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?
| '---->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?
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.