Skip to content

Commit

Permalink
Run ruff as an external tool, not via Python/PEX (#21237)
Browse files Browse the repository at this point in the history
This switches the Ruff subsystem to download and run it via the
artifacts published to GitHub releases.

Ruff is published to PyPI and thus runnable as a PEX, which is what
Pants currently does... but under the hood ruff is a single binary, and
the PyPI package is for convenience. The package leads to a bit of extra
overhead (e.g. have to build a `VenvPex` before being able to run it).
It is also fiddly to change the version of a Python tool, requiring
building a resolve and using `python_requirement`s.

By downloading from GitHub releases, we can:

- more easily support multiple ruff versions and allow users to pin to
one/switch between them with a `version = "..."` (this PR demonstrates
this, by including both 0.6.4 as in `main`, and 0.4.9 as in 2.23, i.e.
if someone upgrades to 2.24 and wants to stick with 0.4.9, they can just
add `version = "0.4.9"`, no need to fiddle with `known_versions`)
- eliminate any Python/pex overhead by invoking the binary directly
- side-step fiddle like interpreter constraints
(#21235 (comment))

Potential issues:

- If Ruff adds plugins via Python in future, maybe we will want it
installed in a venv so that the venv can include those plugins...
astral-sh/ruff#283 seems to be inconclusive
about the direction, so I'm inclined to not worry and deal with it in
future, if it happens.

This PR does:

- Switches the ruff subsystem to be a `TemplatedExternalTool` subclass,
not `PythonToolBase`
- Plumbs through the requisite changes, including: setting up some
`default_known_versions` for the `main` and 2.23 versions (0.6.4 and
0.4.9 respectively), changing how the tool is installed, removing
metadata about interpreter constraints that is no longer relveant or
computable
- Eases the upgrade by adding deprecated instances of the fields
provided by `PythonToolBase`: people may've customised the Ruff version
with `install_from_resolve`. If the field was just removed, running any
pants command will fail on start-up with `Invalid option
'install_from_resolve' under [ruff] in path/to/pants.toml`, which isn't
very helpful. By having the fields exist, we ensure the user gets a
descriptive warning about what to do. NB. the backend is labelled
experimental, so I haven't tried to preserve behaviour, just focused on
ensuring we can show a removal error message to them.
  • Loading branch information
huonw authored Oct 30, 2024
1 parent ddee75f commit f081583
Show file tree
Hide file tree
Showing 12 changed files with 122 additions and 202 deletions.
4 changes: 0 additions & 4 deletions build-support/bin/generate_builtin_lockfiles.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
from pants.backend.python.lint.pydocstyle.subsystem import Pydocstyle
from pants.backend.python.lint.pylint.subsystem import Pylint
from pants.backend.python.lint.pyupgrade.subsystem import PyUpgrade
from pants.backend.python.lint.ruff.subsystem import Ruff
from pants.backend.python.lint.yapf.subsystem import Yapf
from pants.backend.python.packaging.pyoxidizer.subsystem import PyOxidizer
from pants.backend.python.subsystems.debugpy import DebugPy
Expand Down Expand Up @@ -127,9 +126,6 @@ class JvmTool(Tool[JvmToolBase]):
PythonTool(PythonProtobufGrpclibPlugin, "pants.backend.codegen.protobuf.python"),
PythonTool(Pytype, "pants.backend.experimental.python.typecheck.pytype"),
PythonTool(PyOxidizer, "pants.backend.experimental.python.packaging.pyoxidizer"),
# Note - Ruff has two backends (<package>.check and <package>.format).
# Both of these rely on the same resolve underneath so we just pick one here.
PythonTool(Ruff, "pants.backend.experimental.python.lint.ruff.check"),
PythonTool(SemgrepSubsystem, "pants.backend.experimental.tools.semgrep"),
PythonTool(Setuptools, "pants.backend.python"),
PythonTool(SetuptoolsSCM, "pants.backend.python"),
Expand Down
2 changes: 2 additions & 0 deletions docs/notes/2.24.x.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ The kotlin linter, [ktlint](https://pinterest.github.io/ktlint/), has been updat

#### Python

**Breaking change**: the `pants.backend.experimental.python.lint.ruff.check` and `pants.backend.experimental.python.lint.ruff.format` subsystems now execute Ruff as a downloaded binary, directly from the Ruff releases, rather than using PEX to execute the PyPI `ruff` package. This has less overhead, and makes customising the version simpler. However, **options like [`[ruff].install_from_resolve`](https://www.pantsbuild.org/2.24/reference/subsystems/ruff#install_from_resolve) are now ignored** and will be removed in future. If you have customised the version, use [`[ruff].version`](https://www.pantsbuild.org/2.24/reference/subsystems/ruff#version) and [`[ruff].known_versions`](https://www.pantsbuild.org/2.24/reference/subsystems/ruff#known_versions) instead.

User API Changes:
- Update default package mapping for `pymupdf` to match imports from both `fitz` (the legacy name) and `pymupdf` (the [currently supported name](https://pymupdf.readthedocs.io/en/latest/installation.html#problems-after-installation).

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,12 @@
from pants.backend.build_files.fmt.ruff.register import RuffRequest
from pants.backend.build_files.fmt.ruff.register import rules as ruff_build_rules
from pants.backend.python.lint.ruff.check.rules import rules as ruff_fmt_rules
from pants.backend.python.lint.ruff.subsystem import Ruff
from pants.backend.python.lint.ruff.subsystem import rules as ruff_subsystem_rules
from pants.backend.python.target_types import PythonSourcesGeneratorTarget
from pants.core.goals.fmt import FmtResult
from pants.core.util_rules import config_files
from pants.engine.fs import PathGlobs
from pants.engine.internals.native_engine import Snapshot
from pants.testutil.python_interpreter_selection import all_major_minor_python_versions
from pants.testutil.rule_runner import QueryRule, RuleRunner


Expand Down Expand Up @@ -49,16 +47,10 @@ def run_ruff(rule_runner: RuleRunner, *, extra_args: list[str] | None = None) ->


@pytest.mark.platform_specific_behavior
@pytest.mark.parametrize(
"major_minor_interpreter",
all_major_minor_python_versions(Ruff.default_interpreter_constraints),
)
def test_passing(rule_runner: RuleRunner, major_minor_interpreter: str) -> None:
def test_passing(rule_runner: RuleRunner) -> None:
rule_runner.write_files({"BUILD": 'python_sources(name="t")\n'})
interpreter_constraint = f"=={major_minor_interpreter}.*"
fmt_result = run_ruff(
rule_runner,
extra_args=[f"--ruff-interpreter-constraints=['{interpreter_constraint}']"],
)
assert "1 file left unchanged" in fmt_result.stdout
assert fmt_result.output == rule_runner.make_snapshot({"BUILD": 'python_sources(name="t")\n'})
Expand Down
7 changes: 3 additions & 4 deletions src/python/pants/backend/build_files/fmt/ruff/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
from pants.backend.python.lint.ruff.format import rules as ruff_format_backend
from pants.backend.python.lint.ruff.format.rules import _run_ruff_fmt
from pants.backend.python.lint.ruff.subsystem import Ruff
from pants.backend.python.subsystems.python_tool_base import get_lockfile_interpreter_constraints
from pants.core.goals.fmt import FmtResult
from pants.engine.platform import Platform
from pants.engine.rules import collect_rules, rule
from pants.util.logging import LogLevel

Expand All @@ -17,9 +17,8 @@ class RuffRequest(FmtBuildFilesRequest):


@rule(desc="Format with Ruff", level=LogLevel.DEBUG)
async def ruff_fmt(request: RuffRequest.Batch, ruff: Ruff) -> FmtResult:
ruff_ics = await get_lockfile_interpreter_constraints(ruff)
return await _run_ruff_fmt(request, ruff, ruff_ics)
async def ruff_fmt(request: RuffRequest.Batch, ruff: Ruff, platform: Platform) -> FmtResult:
return await _run_ruff_fmt(request, ruff, platform)


def rules():
Expand Down
6 changes: 1 addition & 5 deletions src/python/pants/backend/python/lint/ruff/BUILD
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
# Copyright 2023 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

resource(name="lockfile", source="ruff.lock")

python_sources(
overrides={"subsystem.py": {"dependencies": [":lockfile"]}},
)
python_sources()

python_tests(
name="tests",
Expand Down
9 changes: 7 additions & 2 deletions src/python/pants/backend/python/lint/ruff/check/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
from pants.core.goals.lint import LintResult, LintTargetsRequest
from pants.core.util_rules.partitions import PartitionerType
from pants.core.util_rules.source_files import SourceFiles, SourceFilesRequest
from pants.engine.platform import Platform
from pants.engine.rules import Get, collect_rules, rule
from pants.engine.target import FieldSet, Target
from pants.util.logging import LogLevel
Expand Down Expand Up @@ -72,24 +73,28 @@ def tool_id(cls) -> str:


@rule(desc="Fix with `ruff check --fix`", level=LogLevel.DEBUG)
async def ruff_fix(request: RuffFixRequest.Batch, ruff: Ruff) -> FixResult:
async def ruff_fix(request: RuffFixRequest.Batch, ruff: Ruff, platform: Platform) -> FixResult:
result = await run_ruff(
RunRuffRequest(snapshot=request.snapshot, mode=RuffMode.FIX),
ruff,
platform,
)
return await FixResult.create(request, result)


@rule(desc="Lint with `ruff check`", level=LogLevel.DEBUG)
async def ruff_lint(
request: RuffLintRequest.Batch[RuffCheckFieldSet, Any], ruff: Ruff
request: RuffLintRequest.Batch[RuffCheckFieldSet, Any],
ruff: Ruff,
platform: Platform,
) -> LintResult:
source_files = await Get(
SourceFiles, SourceFilesRequest(field_set.source for field_set in request.elements)
)
result = await run_ruff(
RunRuffRequest(snapshot=source_files.snapshot, mode=RuffMode.LINT),
ruff,
platform,
)
return LintResult.create(request, result)

Expand Down
25 changes: 10 additions & 15 deletions src/python/pants/backend/python/lint/ruff/common.py
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
# Copyright 2024 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).
from dataclasses import dataclass
from typing import Optional, Tuple
from typing import Tuple

from typing_extensions import assert_never

from pants.backend.python.lint.ruff.subsystem import Ruff, RuffMode
from pants.backend.python.util_rules.interpreter_constraints import InterpreterConstraints
from pants.backend.python.util_rules.pex import PexRequest, VenvPex, VenvPexProcess
from pants.core.util_rules.config_files import ConfigFiles, ConfigFilesRequest
from pants.core.util_rules.external_tool import DownloadedExternalTool, ExternalToolRequest
from pants.engine.fs import Digest, MergeDigests
from pants.engine.internals.native_engine import Snapshot
from pants.engine.process import FallibleProcessResult
from pants.engine.platform import Platform
from pants.engine.process import FallibleProcessResult, Process
from pants.engine.rules import Get, MultiGet
from pants.util.logging import LogLevel
from pants.util.strutil import pluralize
Expand All @@ -21,28 +21,24 @@
class RunRuffRequest:
snapshot: Snapshot
mode: RuffMode
interpreter_constraints: Optional[InterpreterConstraints] = None


async def run_ruff(
request: RunRuffRequest,
ruff: Ruff,
platform: Platform,
) -> FallibleProcessResult:
ruff_pex_get = Get(
VenvPex,
PexRequest,
ruff.to_pex_request(interpreter_constraints=request.interpreter_constraints),
)
ruff_tool_get = Get(DownloadedExternalTool, ExternalToolRequest, ruff.get_request(platform))

config_files_get = Get(
ConfigFiles, ConfigFilesRequest, ruff.config_request(request.snapshot.dirs)
)

ruff_pex, config_files = await MultiGet(ruff_pex_get, config_files_get)
ruff_tool, config_files = await MultiGet(ruff_tool_get, config_files_get)

input_digest = await Get(
Digest,
MergeDigests((request.snapshot.digest, config_files.snapshot.digest)),
MergeDigests((ruff_tool.digest, request.snapshot.digest, config_files.snapshot.digest)),
)

conf_args = [f"--config={ruff.config}"] if ruff.config else []
Expand All @@ -64,9 +60,8 @@ async def run_ruff(

result = await Get(
FallibleProcessResult,
VenvPexProcess(
ruff_pex,
argv=(*initial_args, *conf_args, *ruff.args, *request.snapshot.files),
Process(
argv=(ruff_tool.exe, *initial_args, *conf_args, *ruff.args, *request.snapshot.files),
input_digest=input_digest,
output_files=request.snapshot.files,
description=f"Run ruff {' '.join(initial_args)} on {pluralize(len(request.snapshot.files), 'file')}.",
Expand Down
12 changes: 5 additions & 7 deletions src/python/pants/backend/python/lint/ruff/format/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from dataclasses import dataclass
from typing import Optional

from pants.backend.python.lint.ruff.common import RunRuffRequest, run_ruff
from pants.backend.python.lint.ruff.format.skip_field import SkipRuffFormatField
Expand All @@ -14,9 +13,9 @@
PythonSourceField,
)
from pants.backend.python.util_rules import pex
from pants.backend.python.util_rules.interpreter_constraints import InterpreterConstraints
from pants.core.goals.fmt import AbstractFmtRequest, FmtResult, FmtTargetsRequest
from pants.core.util_rules.partitions import PartitionerType
from pants.engine.platform import Platform
from pants.engine.rules import collect_rules, rule
from pants.engine.target import FieldSet, Target
from pants.util.logging import LogLevel
Expand Down Expand Up @@ -55,20 +54,19 @@ def tool_id(self) -> str:
async def _run_ruff_fmt(
request: AbstractFmtRequest.Batch,
ruff: Ruff,
interpreter_constraints: Optional[InterpreterConstraints] = None,
platform: Platform,
) -> FmtResult:
run_ruff_request = RunRuffRequest(
snapshot=request.snapshot,
mode=RuffMode.FORMAT,
interpreter_constraints=interpreter_constraints,
)
result = await run_ruff(run_ruff_request, ruff)
result = await run_ruff(run_ruff_request, ruff, platform)
return await FmtResult.create(request, result)


@rule(desc="Format with `ruff format`", level=LogLevel.DEBUG)
async def ruff_fmt(request: RuffFormatRequest.Batch, ruff: Ruff) -> FmtResult:
return await _run_ruff_fmt(request, ruff)
async def ruff_fmt(request: RuffFormatRequest.Batch, ruff: Ruff, platform: Platform) -> FmtResult:
return await _run_ruff_fmt(request, ruff, platform)


def rules():
Expand Down
139 changes: 0 additions & 139 deletions src/python/pants/backend/python/lint/ruff/ruff.lock

This file was deleted.

Loading

0 comments on commit f081583

Please sign in to comment.