Skip to content

Commit

Permalink
Major Refactor: Add Ruff Linting, Nox, and Pre-Commit Hook (#266)
Browse files Browse the repository at this point in the history
* added ruff and nox.

* config nox for the py3.12

* add `nox`, `ruff` and a section on managing multiple python versions

* adds src to ruff config

* updates ruff version (to work with src config)

* added ruff and pre-commit configurations

* basic precommit confi

* ruff linter added to ci

* formatter added

* default check added to ci

* assigned separate github actions to ruff code formatter and linters

* test

* test

* ruff badge added to readme

* noxfile added for automated testing across environments after commits

* ruff linters, both ignore and select, specified

* ruff check runs

* toml formatting

* added addtional pre-commit hooks

* pre-commit edits

* default for max line length is 88

* error in deploy file: pypi environment not defined

* link changed to previous one in ci.yml

* ruff linting and formatting combined in 1 action in CI and added to needs field in check

* ci -- calling ruff directly instead of via action

* test

* .

* null changes in notebook reverted

* in ci, specifying to use ruff config from pyproject.toml

* updating some deprecated imports, isinstance for union of types, unsorted imports, f-strings, replaced single quote with double quotes and deleted trailing whitespace

* notebooks 00-quickstart and 02-eigendistortions line-length correction

* Revert "notebooks 00-quickstart and 02-eigendistortions line-length correction"

This reverts commit 4bb4333.

* Revert "updating some deprecated imports, isinstance for union of types, unsorted imports, f-strings, replaced single quote with double quotes and deleted trailing whitespace"

This reverts commit c1fd8bc.

* additional line length and double quote fixes in 02-eigendistortions.ipynb

* all notebooks in experiments refactored to meet pydocstyle and pyflakes criteria

* all files in metric  refactored to meet pydocstyle and pyflakes criteria

* all files in simulate and models  refactored to meet pydocstyle and pyflakes criteria

* all files in src refactored to meet pydocstyle and pyflakes criteria

* running pyupgrade linter to upgrade syntac for newer versions

* running flake8 simplify on entire codebase

* removing  # noqa: UP035 tag

* test coverage session added to noxfile

* pytest ini_options adjustments to accomodate module not implemented error when runnin nox sesssion tests

* updating test session in nox file to install all dependencies as listes in toml file and fixing none-type error in eigendistortions.py

* pytest can now be run with nox including test coverage

* resolving some too long lines

* too long lines in validate.py corrected

* formatting with line-length set to 88

* too long lines shortened to 88 characters in data and metric

* .

* some more fixes

* removed .keys() from dictionary iteration, replaced if-else with 1-liners, used f-strings, shortened if statements with boolean expressions, added contextlib for with statements, and refactored lambda expressions into methods.

* ignored SIM105 or do we want to use contextlib package instead of try-except-pass blocks?

* ambigious variable name in external refactored

* tests test_metric and test_models fail

* replacing if-else block with ternary conditional operator

* removing too long lines

* replacing if-else block by returning boolean expression directly

* returning boolean expression as opposed to if-if-else block

* replacing nested if-else blocks with elif for readability

* too long lines fixed

* in optimizer_step in metamer.py condensed if statements and added check for index out of bound

* simplified decision tree conditions in check_convergence

* metamers.py refactoring finnished and all tests in test_metamers.py pass

* checking if module is available without importing it unnecessarily

* ignoring import not being at top of file for fetch.py

* updating union syntax to python 3.10 bar version, unrelated 420 sha-errors and 2 failed tests asserting x < some threshold

* ignoring unused imports linting error F401 in tools init file

* ignoring unused imports linting error F401  in metric init file

* ignoring wildcard imports  linting error F403  in canonical computations  init file

* ignoring wildcard imports  linting error F403 and unused imports F401  in tools  init file

* added predicate ignore-init-module-imports to tool.ruff.lint in pyproject.toml and set it to true

* removed predicate ignore-init-module-imports since deprecated

* ignoring unused imports F401 and wildcard imports F403

* ignoring unused imports F401 in synthesize init file

* ignoring wildcard  imports F401 in simlute init file

* resolving linting error E402 imports not at top of cell

* replacing union with pipe operator which resolves UP007 in steerable_pyramid_freq.py

* cutting line lenght

* making if-blocks more readable and cutting long lines in mad_comptetition.py

* placing imports to top of cell and shortening too long lines

* fixing too long lines and placing imports at top of cell in notebookds 08 and 06.

* reordering imports and fixing too long lines in notebook geodesics

* .

* shortening lines in eigendistortions.py

* too long lines fixed in notebook metamer-portilla-simoncelli

* too long lines fixed in notebook metamer-portilla-simoncelli

* too long lines fixed in notebooks

* too long lines fixed in notebooks

* too long lines fixed in notebooks

* too long lines fixed in notebooks

* too long lines fixed in notebooks

* replacing misleading characters l

* replacing ambigious l with layer

* shortening lines

* replacing ambigious variable names

* .

* fixing too long lines

* ignoring too long lines in notebook 05 does not work for curl statement

* added isort linter and fixed 44 isort errors

* reformatting

* tests run again and circular import error resolved

* adding missing import data to init file

* fixing linting error

* ruff version updated to 0.6.8, same as on cluster

* adding missing import conv to init in tools

* formatting fix

* editing string to mitigate notebook error for versions 3.10 and 3.11

* updated 3 cns links from http to https

* imports sorted in init

* isort ignored in init file, otherwise errors due to circulr inputs

* unused imports uncommented

* when unused imports are removed, tests fail, so included them again

* reformatting tests and updating contributing file

* contributing file update completed

* deleting pypi environment in deploy.yml which slipped in from a merge but doesn't belong into this PR

* Update CONTRIBUTING.md

Co-authored-by: William F. Broderick <billbrod@gmail.com>

* Update CONTRIBUTING.md

Co-authored-by: William F. Broderick <billbrod@gmail.com>

* Update CONTRIBUTING.md

Co-authored-by: William F. Broderick <billbrod@gmail.com>

* Update CONTRIBUTING.md

Co-authored-by: William F. Broderick <billbrod@gmail.com>

* Update CONTRIBUTING.md

Co-authored-by: William F. Broderick <billbrod@gmail.com>

* Update CONTRIBUTING.md

Co-authored-by: William F. Broderick <billbrod@gmail.com>

* Update CONTRIBUTING.md

Co-authored-by: William F. Broderick <billbrod@gmail.com>

* Update CONTRIBUTING.md

Co-authored-by: William F. Broderick <billbrod@gmail.com>

* Update CONTRIBUTING.md

Co-authored-by: William F. Broderick <billbrod@gmail.com>

* replacing os.path.join with pathlib and / operator to concatenate paths

* removing ignore in nox lint session

* nox removed from mandatory installments

* moved import pathlib to top of fetch.py file

* adding two singleton dimensinos using the unsqueeze function twice, replacing the None, None expression, for better readability

* added examples to tool.ruff in pyproject.toml

* removed pydocstyle inting from pyproject.toml as we are currently not using this linter

* Update CONTRIBUTING.md

Co-authored-by: William F. Broderick <billbrod@gmail.com>

* Update CONTRIBUTING.md

Co-authored-by: William F. Broderick <billbrod@gmail.com>

* pyproject.toml parse error for ruff linter, to be fixed

* ruff actions on ci should work now, removed numpy convention, to be added in separeate PR where the pydocstyle linter will be added

* fixed SIM105 errors, i.e., replaced try-except blocks using contextlib and specifc error that will be raised

* reverting flowcharts to original version with too long lines and ignored too long lines

* removed file extension to be excluded by ruff as they should be ignored through gitignore

* replace assert with if/raise

looks cleaner

* mention pre-commit.ci in contributing

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* linting tests and import order

* added explicit isort skips to imports which need to be in a specific order

* fixing failing tests by making it catch Value error as opposed to assertion error

* fixing too long lines in tests

* lambda functions assigned to variables are replaced by acutal function definitions in test validate metrics. Resolved E731 linting error

* removing unused variables in test_models.py in TestNaive class resolving F841

* removing .keys() when iterating over dictionary keys resolving SIM118

* replacing if-else blocks with ternary operator resolving SIM108

* resolving SIM105 by using contextlib instead of try except block

* replacing percentage sign in print statements with format

* replacing lambda assigned to variable with function

* replacing for loop keeping track of each index with enumerate resolving SIM113

* resoving 2 failed tests by calling model on image in test_models

* removing unused variabl assignments in test_models

* simplifying counter variable in enumeration loop

* updating contributing file: introducing examples for ruff noqa and adding action comments for ruff to disable linting rules

* move cones/primaries to json file

* run ruff on conceptual intro

* remove unnecessary isort:skip

* puts """ on newline

* be consistent with contextlib.suppress

* more changes for suppress

* switch nox test install to use ".[dev]"

* Apply suggestions from code review

---------

Co-authored-by: BalzaniEdoardo <edoardo.balzani87@gmail.com>
Co-authored-by: William F. Broderick <billbrod@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
  • Loading branch information
4 people authored Oct 21, 2024
1 parent d8270ca commit d224d06
Show file tree
Hide file tree
Showing 77 changed files with 7,163 additions and 4,206 deletions.
36 changes: 36 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,40 @@ jobs:
print_all: false
timeout: 5
retry_count: 3
ruff_linting:
runs-on: ubuntu-latest
name: Run Ruff linter
steps:
- uses: actions/checkout@v4
- name: Install Python 3
uses: actions/setup-python@v5
with:
python-version: 3.12
cache: pip
cache-dependency-path: setup.py
- name: Install dependencies
run: |
pip install --upgrade --upgrade-strategy eager .[dev]
- name: Run ruff linter
run: |
ruff check --config=pyproject.toml
ruff_formatting:
runs-on: ubuntu-latest
name: Run Ruff code formatting check
steps:
- uses: actions/checkout@v4
- name: Install Python 3
uses: actions/setup-python@v5
with:
python-version: 3.12
cache: pip
cache-dependency-path: setup.py
- name: Install dependencies
run: |
pip install --upgrade --upgrade-strategy eager .[dev]
- name: Run ruff formatter
run: |
ruff format --check --config=pyproject.toml
check:
if: always()
Expand All @@ -130,6 +164,8 @@ jobs:
- all_tutorials_in_docs
- no_extra_nblinks
- check_urls
- ruff_linting
- ruff_formatting
runs-on: ubuntu-latest
steps:
- name: Decide whether all tests and notebooks succeeded
Expand Down
1 change: 0 additions & 1 deletion .github/workflows/deploy.yml
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ jobs:
publish:
name: Upload release to Test PyPI
needs: [build]
environment: pypi
runs-on: ubuntu-latest
permissions:
id-token: write # IMPORTANT: this permission is mandatory for trusted publishing
Expand Down
31 changes: 31 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
repos:
- repo: https://github.com/astral-sh/ruff-pre-commit
rev: v0.6.8
hooks:
# Run the formatter.
- id: ruff-format
args: [--config=pyproject.toml]
# Run the linter.
- id: ruff
args: [--config=pyproject.toml]


- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v2.3.0
# note: pre-commit runs top-to-bottom, so put the hooks that modify content first,
# followed by checks that might be more likely to pass after the modifactaion hooks (like flake8)
hooks:
# Checks for large files added to the repository, typically to prevent accidental inclusion of large binaries or datasets.
- id: check-added-large-files
# Detects potential filename conflicts due to case-insensitive filesystems (e.g., Windows) where File.txt and file.txt would be considered the same.
- id: check-case-conflict
# Checks for files that contain merge conflict strings (e.g., <<<<<<<, =======, >>>>>>>).
- id: check-merge-conflict
# Validates YAML files for syntax errors.
- id: check-yaml
# Detects debug statments (e.g., print, console.log, etc.) left in code.
- id: debug-statements
# Ensures files have a newline at the end.
- id: end-of-file-fixer
# Removes trailing whitespace characters from files.
- id: trailing-whitespace
1 change: 0 additions & 1 deletion CODE_OF_CONDUCT.md
Original file line number Diff line number Diff line change
Expand Up @@ -132,4 +132,3 @@ conduct enforcement ladder](https://github.com/mozilla/diversity).
For answers to common questions about this code of conduct, see the
FAQ at https://www.contributor-covenant.org/faq. Translations are
available at https://www.contributor-covenant.org/translations.

237 changes: 227 additions & 10 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ git pull upstream main
# update your fork's main branch with any changes from upstream
git push origin main
# create and switch to the branch
git checkout -b my_cool_branch
git checkout -b my_cool_branch
```

Then, create new changes on this branch and, when you're ready, add and commit them:
Expand All @@ -109,24 +109,89 @@ Then, create new changes on this branch and, when you're ready, add and commit t
# stage the changes
git add src/plenoptic/the_file_you_changed.py
# commit your changes
git commit -m "A helpful message explaining my changes"
git commit -m "A helpful message explaining my changes"
# push to the origin remote
git push origin my_cool_branch
git push origin my_cool_branch
```

If you aren't comfortable with `git add`, `git commit`, `git push`, I recommend the [Software Carpentry git lesson](https://swcarpentry.github.io/git-novice/).

#### Contributing your change back to plenoptic

You can make any number of changes on your branch. Once you're happy with your changes, [add tests](#adding-tests) to check that they run correctly and [add documentation](#adding-documentation), then make sure the existing [tests](#testing) all run successfully, that your branch is up-to-date with main, and then open a pull request by clicking on the big `Compare & pull request` button that appears at the top of your fork after pushing to your branch (see [here](https://intersect-training.org/collaborative-git/03-pr/index.html) for a tutorial).
You can make any number of changes on your branch. Once you're happy with your changes, [add tests](#adding-tests) to check that they run correctly and [add documentation](#adding-documentation), then make sure the existing [tests](#testing) all run successfully, that your branch is up-to-date with main, and then open a pull request by clicking on the big `Compare & pull request` button that appears at the top of your fork after pushing to your branch (see [here](https://intersect-training.org/collaborative-git/03-pr/index.html) for a tutorial).

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 Style 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. We run ruff as part of our CI and non-compliant code will not be merged!

#### 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 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.

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:

`ruff format` is the primary entrypoint to the formatter. It accepts a list of files or directories, and formats all discovered Python files:
```bash
ruff format # Format all files in the current directory.
ruff format path/to/code/ # Format all files in `path/to/code` (and any subdirectories).
ruff format path/to/file.py # Format a single file.
```
For the full list of supported options, run `ruff format --help`.

##### Using Ruff for Linting:

To run Ruff on your code:
```bash
ruff check .
```
It'll then tell you which lines are violating linting rules and may suggest that some errors are automatically fixable.

To automatically fix lintint errors, run:

```bash
ruff --fix .
```

Be careful with **unsafe fixes**, safe fixes are symbolized with the tools emoji and are listed [here](https://docs.astral.sh/ruff/rules/)!

#### Ignoring Ruff Linting
In some cases, it may be acceptable to suppress lint errors, for example when too long lines (code `E501`) are desired because otherwise the url might not be readable anymore. These ignores will be evaluated on a case-by-case basis.
You can do this by adding the following to the end of the line:

```bash
This line is tooooooo long. # noqa: E501
```
If you want to suppress an error across an entire file, do this at the top of the file:
```bash
# ruff: noqa: E501
Below is my python script
...
...
And any line living in this file can be as long as it wants ...
...
```


In some cases, you want to not only suppress the error message a linter throws but actually _disable_ a linting rule. An example might be if the import order matters and running `isort` would mess with this.
In these cases, you can introduce an [action comment](https://docs.astral.sh/ruff/linter/#action-comments) like this such that ruff does _not_ sort the following packages alphabetically:

```bash
import numpy as np # isort: skip
import my_package as mp # isort: skip
```

For more details, refer to the [documentation](https://docs.astral.sh/ruff/linter/#error-suppression).

#### General Style Guide Recommendations:

- Longer, descriptive names are preferred (e.g., `x` is not an appropriate name
for a variable), especially for anything user-facing, such as methods,
Expand All @@ -135,6 +200,26 @@ If your changes are integrated, you will be added as a Github contributor and as
(see [below](#docstrings) for details). Hidden ones do not *need* to have
complete docstrings, but they probably should.

#### Pre-Commit Hooks: Identifying simple issues before submission to code review (and how to ignore those)
[Pre-commit](https://pre-commit.com/) hooks are useful for the developer to check if all the linting and formatting rules (see Ruff above) are honored _before_ committing. That is, when you commit, pre-commit hooks are run and auto-fixed where possible (e.g., trailing whitespace). You then need to add _again_ if you want these changes to be included in your commit. If the problem is not automatically fixable, you will need to manually update your code before you are able to commit.

Using pre-commit is optional. We use [pre-commit.ci](https://pre-commit.ci/) to run pre-commit as part of PRs (auto-fixing wherever possible), but it may simplify your life to integrate pre-commit into your workflow.

In order to use pre-commit, you must install the `pre-commit` package into your development environment, and then install the hooks:

```bash
pip install pre-commit
pre-commit install
```

See [pre-commit docs](https://pre-commit.com/) for more details.

After installation, should you want to ignore pre-commit hooks for some reason (e.g., because you have to run to a meeting and so don't have time to fix all the linting errors but still want your changes to be commited), you can add `--no-verify` to your commit message like this:
```bash
git commit -m <my commit message> --no-verify
```


### Adding models or synthesis methods

In addition to the above, see the documentation for a description of
Expand Down Expand Up @@ -199,7 +284,139 @@ several choices for how to run a subset of the tests:
View the [pytest documentation](https://doc.pytest.org/en/latest/usage.html) for
more info.

### Adding tests
### Using nox to simplify testing and linting
This section is optional but if you want to easily run tests in an isolated environment
using the [nox](https://nox.thea.codes/en/stable/) command-line tool.

`nox` is installed automatically as a `[dev]` dependency of plenoptic.

To run all tests and linters through `nox`, from the root folder of the
plenoptic package, execute the following command,

```bash
nox
```

`nox` will read the configuration from the `noxfile.py` script.

If you only want to run an individual session (e.g., lint or test), you can first check which sessions are available with the following command:

```bash
nox -l
```

Then you can use

```bash
nox -s <your_nox_session>
```
to run the session of your choice.

Here are some examples:

If you want to run just the tests:

```bash
nox -s tests
```

for running only the linters,

```bash
nox -s linters
```

and for testing only the coverage, run:

```bash
nox -s coverage
```

`nox` offers a variety of configuration options, you can learn more about it from their
[documentation](https://nox.thea.codes/en/stable/config.html).

Note that nox works particularly well with pyenv, discussed later in this file, which makes it easy to install the multiple python versions used in testing.

#### Multi-python version testing with pyenv
Sometimes, before opening a pull-request that will trigger the `.github/workflow/ci.yml` continuous
integration workflow, you may want to test your changes over all the supported python versions locally.

Handling multiple installed python versions on the same machine can be challenging and confusing.
[`pyenv`](https://github.com/pyenv/pyenv) is a great tool that really comes to the rescue. Note that `pyenv` just handles python versions --- virtual environments have to be handled separately, using [`pyenv-virtualenv`](https://github.com/pyenv/pyenv-virtualenv)!

This tool doesn't come with the package dependencies and has to be installed separately. Installation instructions
are system specific but the package readme is very details, see
[here](https://github.com/pyenv/pyenv?tab=readme-ov-file#installation).

Follow carefully the instructions to configure pyenv after installation.

Once you have tha package installed and configured, you can install multiple python version through it.
First get a list of the available versions with the command,

```bash
pyenv install -l
```

Install the python version you need. For this example, let's assume we want `python 3.10.11` and `python 3.11.8`,

```bash
pyenv install 3.10.11
pyenv install 3.11.8
```

You can check which python version is currently set as default, by typing,

```bash
pyenv which python
```

And you can list all available versions of python with,

```bash
pyenv versions
```
If you want to run `nox` on multiple python versions, all you need to do is:

1. Set your desired versions as `global`.
```bash
pyenv global 3.11.8 3.10.11
```
This will make both version available, and the default python will be set to the first one listed
(`3.11.8` in this case).
2. Run nox specifying the python version as an option.
```bash
nox -p 3.10
```

Note that `noxfile.py` lists the available option as keyword arguments in a session specific manner.

As mentioned earlier, if you have multiple python version installed, we recommend you manage your virtual environments through `pyenv` using the [`pyenv-virtualenv`](https://github.com/pyenv/pyenv-virtualenv) extension.

This tool works with most of the environment managers including (`venv` and `conda`).
Creating an environment with it is as simple as calling,

```bash
pyenv virtualenv my-python my-enviroment
```

Here, `my-python` is the python version, one between `pyenv versions`, and `my-environment` is your
new environment name.

If `my-python` has `conda` installed, it will create a conda environment, if not, it will use `venv`.

You can list the virtual environment only with,

```bash
pyenv virtualenvs
```

And you can uninstall an environment with,

```bash
pyenv uninstall my-environment
```

### Adding tests

New tests can be added in any of the existing `tests/test_*.py` scripts. Tests
should be functions, contained within classes. The class contains a bunch of
Expand Down Expand Up @@ -261,7 +478,7 @@ metamer instances run for. We do this using
`vgg16_synth_max_iter 10`; note that you need a `-p` for each parameter and
you should change nothing else about that line). See the block with `if: ${{
matrix.notebook == 'examples/Demo_Eigendistortion.ipynb' }}` for an example.
A similar procedure could be used to reduce the size of an image or other steps
that could similarly reduce the total time necessary to run a notebook.
Expand Down Expand Up @@ -395,7 +612,7 @@ header (you can have as many sub-headers as you'd like).
You should [build the docs yourself](#build-the-documentation) to ensure it
looks correct before pushing.
#### Images and plots
You can include images in `.rst` files in the documentation as well. Simply
Expand All @@ -405,9 +622,9 @@ place them in the `docs/images/` folder and use the `figure` role, e.g.,:
.. figure:: images/path_to_my_image.svg
:figwidth: 100%
:alt: Alt-text describing my image.
Caption describing my image.
```
To refer to it directly, you may want to use the [numref
Expand Down
Loading

0 comments on commit d224d06

Please sign in to comment.