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

Use ruff format as our Python formatter #3839

Merged
merged 18 commits into from
Oct 26, 2023
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions .github/workflows/contrib_checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ permissions:

jobs:
py-lints:
name: Python lints (black, mypy, flake8)
name: Python lints (ruff, mypy, )
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
Expand Down Expand Up @@ -88,6 +88,11 @@ jobs:
# PR introduces a new type and another PR changes the codegen.
- uses: actions/checkout@v4

# So we can format the Python code
- name: Set up Python
run: |
pip install -r rerun_py/requirements-lint.txt

- name: Codegen
uses: actions-rs/cargo@v1
with:
Expand Down Expand Up @@ -258,7 +263,7 @@ jobs:
id: expected_version
run: ./scripts/ci/cargo_deny.sh

cpp-formating:
cpp-formatting:
name: C++ formatting check
runs-on: ubuntu-latest
steps:
Expand Down
9 changes: 7 additions & 2 deletions .github/workflows/reusable_checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ jobs:
# ---------------------------------------------------------------------------

py-lints:
name: Python lints (black, mypy, flake8)
name: Python lints (ruff, mypy, )
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
Expand Down Expand Up @@ -130,6 +130,11 @@ jobs:
workload_identity_provider: ${{ secrets.GOOGLE_WORKLOAD_IDENTITY_PROVIDER }}
service_account: ${{ secrets.GOOGLE_SERVICE_ACCOUNT }}

# So we can format the Python code
- name: Set up Python
run: |
pip install -r rerun_py/requirements-lint.txt

- name: Codegen
uses: actions-rs/cargo@v1
with:
Expand Down Expand Up @@ -350,7 +355,7 @@ jobs:

# ---------------------------------------------------------------------------

cpp-formating:
cpp-formatting:
name: C++ formatting check
runs-on: ubuntu-latest
steps:
Expand Down
1 change: 0 additions & 1 deletion .vscode/extensions.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
"gaborv.flatbuffers",
"github.vscode-github-actions",
"josetr.cmake-language-support-vscode",
"ms-python.black-formatter",
"ms-python.mypy-type-checker",
"ms-python.python",
"ms-vscode.cmake-tools",
Expand Down
4 changes: 1 addition & 3 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,7 @@
"cmake.buildDirectory": "${workspaceRoot}/build/",
"cmake.generator": "Ninja", // Use Ninja, just like we do in our just/pixi command.
"rust-analyzer.showUnlinkedFileNotification": false,
"black-formatter.args": [
"--config=rerun_py/pyproject.toml"
],
"editor.defaultFormatter": "charliermarsh.ruff",
"ruff.format.args": [
"--config=rerun_py/pyproject.toml"
],
Expand Down
2 changes: 1 addition & 1 deletion crates/re_types_builder/src/codegen/python.rs
Original file line number Diff line number Diff line change
Expand Up @@ -816,7 +816,7 @@ fn code_for_union(

// Note: mypy gets confused using staticmethods for field-converters
code.push_text(
format!("inner: {inner_type} = field({converter}) {type_ignore}"),
format!("inner: {inner_type} = field({converter} {type_ignore}\n)"),
1,
4,
);
Expand Down
25 changes: 11 additions & 14 deletions crates/re_types_builder/src/format/python.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,11 @@ impl CodeFormatter for PythonCodeFormatter {

re_tracing::profile_function!();

// Running `black` once for each file is very slow, so we write all
// Back when we used `black` for formatting, it was very slow
// to run it for each file. So we write all
// files to a temporary folder, format it, and copy back the results.
// Now that we use `ruff format` we could probably revisit this and instead
// format each file individually, in parallel.

let tempdir = tempfile::tempdir().unwrap();
let tempdir_path = Utf8PathBuf::try_from(tempdir.path().to_owned()).unwrap();
Expand Down Expand Up @@ -78,16 +81,9 @@ fn format_path_for_tmp_dir(
fn format_python_dir(dir: &Utf8PathBuf) -> anyhow::Result<()> {
re_tracing::profile_function!();

// The order below is important and sadly we need to call black twice. Ruff does not yet
// fix line-length (See: https://github.com/astral-sh/ruff/issues/1904).
//
// 1) Call black, which among others things fixes line-length
// 2) Call ruff, which requires line-lengths to be correct
// 3) Call black again to cleanup some whitespace issues ruff might introduce

run_black_on_dir(dir).context("black")?;
run_ruff_on_dir(dir).context("ruff")?;
run_black_on_dir(dir).context("black")?;
emilk marked this conversation as resolved.
Show resolved Hide resolved
// NOTE: the order here matches the one in `justfile`:
run_ruff_fix_on_dir(dir).context("ruff --fix")?;
run_ruff_format_on_dir(dir).context("ruff --format")?;
Ok(())
}

Expand All @@ -99,11 +95,12 @@ fn python_project_path() -> Utf8PathBuf {
path
}

fn run_black_on_dir(dir: &Utf8PathBuf) -> anyhow::Result<()> {
fn run_ruff_format_on_dir(dir: &Utf8PathBuf) -> anyhow::Result<()> {
re_tracing::profile_function!();
use std::process::{Command, Stdio};

let proc = Command::new("black")
let proc = Command::new("ruff")
.arg("format")
.arg(format!("--config={}", python_project_path()))
.arg(dir)
.stdout(Stdio::piped())
Expand All @@ -121,7 +118,7 @@ fn run_black_on_dir(dir: &Utf8PathBuf) -> anyhow::Result<()> {
}
}

fn run_ruff_on_dir(dir: &Utf8PathBuf) -> anyhow::Result<()> {
fn run_ruff_fix_on_dir(dir: &Utf8PathBuf) -> anyhow::Result<()> {
re_tracing::profile_function!();
use std::process::{Command, Stdio};

Expand Down
12 changes: 3 additions & 9 deletions justfile
Original file line number Diff line number Diff line change
Expand Up @@ -101,15 +101,9 @@ py-build *ARGS:
py-format:
#!/usr/bin/env bash
set -euo pipefail
# The order below is important and sadly we need to call black twice. Ruff does not yet
# fix line-length (See: https://github.com/astral-sh/ruff/issues/1904).
#
# 1) Call black, which among others things fixes line-length
# 2) Call ruff, which requires line-lengths to be correct
# 3) Call black again to cleanup some whitespace issues ruff might introduce
black --config rerun_py/pyproject.toml {{py_folders}}
# NOTE: the order here matches the one in `crates/re_types_builder/src/format/python.rs`:
ruff --fix --config rerun_py/pyproject.toml {{py_folders}}
black --config rerun_py/pyproject.toml {{py_folders}}
ruff format --config rerun_py/pyproject.toml {{py_folders}}
Copy link
Member

Choose a reason for hiding this comment

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

is there a deeper story that this isn't a pixi task yet? There is a pixi task for checking but not for formatting 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, this is the deeper story: #3717 :)

Copy link
Member Author

Choose a reason for hiding this comment

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

There is also the {[py_folders}} which I don't know if pixi supports. Anyway, this PR is not about moving more tasks into pixi

blackdoc {{py_folders}}
emilk marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

this seems to imply that there is still need for black? But it was removed from the pixi toml and everywhere else 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

blackdoc formats code in docstrings, which ruff format does not yet do: astral-sh/ruff#7146

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a comment about this


# Check that all the requirements.txt files for all the examples are correct
Expand All @@ -123,7 +117,7 @@ py-lint:
#!/usr/bin/env bash
set -euxo pipefail
ruff check --config rerun_py/pyproject.toml {{py_folders}}
black --check --config rerun_py/pyproject.toml --diff {{py_folders}}
ruff format --check --config rerun_py/pyproject.toml {{py_folders}}
blackdoc --check {{py_folders}}
mypy --no-warn-unused-ignore

Expand Down
58 changes: 30 additions & 28 deletions pixi.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 3 additions & 4 deletions pixi.toml
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ lint-cpp-files = "clang-format --dry-run"
lint-rerun = "python scripts/lint.py"
lint-rs-files = "rustfmt --check"
lint-rs-all = "cargo fmt --check"
lint-py-black = "black --check --config rerun_py/pyproject.toml"
lint-py-fmt-check = "ruff format --check --config rerun_py/pyproject.toml"
lint-py-blackdoc = "blackdoc --check"
lint-py-mypy = "mypy --install-types --non-interactive --no-warn-unused-ignore"
lint-py-mypy = "mypy --no-warn-unused-ignore"
emilk marked this conversation as resolved.
Show resolved Hide resolved
lint-py-ruff = "ruff check --config rerun_py/pyproject.toml"
lint-taplo = "taplo fmt --check"
lint-typos = "typos"
Expand Down Expand Up @@ -70,7 +70,6 @@ cpp-test = { cmd = "export RERUN_STRICT=1 && ./build/rerun_cpp/tests/rerun_sdk_t
[dependencies]
arrow-cpp = "10.0.1"
attrs = ">=23.1.0"
black = "23.3.0"
blackdoc = "0.3.8"
c-compiler = "1.6.0.*"
clang = ">=15,<16"
Expand All @@ -89,7 +88,7 @@ pyarrow = "10.0.1"
pytest = ">=7"
python = ">=3.8,<3.12"
python-frontmatter = ">=1.0.0"
ruff = "0.0.276"
ruff = "0.1.2"
semver = ">=2.13,<2.14"
taplo = ">=0.8.1"
typing_extensions = ">4.5"
Expand Down
6 changes: 2 additions & 4 deletions rerun_py/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,6 @@ repository = "https://github.com/rerun-io/rerun"
[project.scripts]
rerun = "rerun.__main__:main"

[tool.black]
line-length = 120
target-version = ["py38"]

[tool.ruff]
# https://beta.ruff.rs/docs/configuration/

Expand Down Expand Up @@ -90,6 +86,8 @@ ignore = [

# allow relative imports
"TID252",

"UP007", # We need this, or `ruff format` will convert `Union[X, Y]` to `X | Y` which break on Python 3.8
]

line-length = 120
Expand Down
3 changes: 1 addition & 2 deletions rerun_py/requirements-lint.txt
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
attrs>=23.1.0 # for mypy to work
black==23.3.0
blackdoc==0.3.8
mypy==1.4.1
numpy>=1.24 # For mypy plugin
torch>=2.1.0
pip-check-reqs==2.4.4 # Checks for missing deps in requirements.txt files
pytest # For mypy to work
ruff==0.0.276
ruff==0.1.2
types-Deprecated==1.2.9.2
types-requests>=2.31,<3
2 changes: 1 addition & 1 deletion rerun_py/rerun_sdk/rerun/datatypes/annotation_info.py

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion rerun_py/rerun_sdk/rerun/datatypes/material.py

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion rerun_py/rerun_sdk/rerun/datatypes/rotation3d.py

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading