Run ruff as an external tool, not via Python/PEX #21237

huonw · 2024-07-31T03:25:40Z

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

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 (Update ruff to version 0.5.5 #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... Meta issue: plugin system 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.

huonw · 2024-07-31T03:26:39Z

src/python/pants/backend/python/lint/ruff/rules_integration_test.py

-    assert lint_result.exit_code == 1
+    assert lint_result.exit_code == 0


I haven't yet done any investigation to establish why this behaviour changes, but should

Ah, the old behaviour was incorrect. The new behaviour is correct. This test ends up invoking ruff check with no file arguments (because they're all skipped), which means ruff does its own directory walking to find what to lint.

Previously, when using a venv pex: there were a few python files within the ruff pex included in the input digest/snapshot, that the auto-traversal was finding: ruff.pex/__main__.py, ruff.pex/__pex__/__init__.py. These files fail linting (e.g. ruff.pex/__main__.py:12:24: F401 [*] `typing.Any` imported but unused), and thus the lint ruff check invocation fails.

Now, when using the binary directly: there's no python files in the sandbox, so the auto-traversal finds nothing and thus the ruff check invocation succeeds (although has a warning about not finding any Python files)

So, calling this good enough ✅

Interesting. Since the test is intended to check that the files get skipped (which it certainly looks like they do), I agree with you and think the new behavior is clearer.

huonw · 2024-07-31T03:30:45Z

I'm putting this up for feedback before doing the remaining todo items to validate the direction: is there some important reason for installing Ruff via Python/pex that I've missed?

krishnan-chandra · 2024-07-31T11:20:30Z

src/python/pants/backend/python/lint/ruff/subsystem.py

+        "0.4.9|macos_arm64|5f4506d7ec2ae6ac5a48ba309218a4b825a00d4cad9967b7bbcec1724ef04930|8148128|https://github.com/astral-sh/ruff/releases/download/v0.4.9/ruff-0.4.9-aarch64-apple-darwin.tar.gz",
+        "0.4.9|macos_x86_64|e4d745adb0f5a0b08f2c9ca71e57f451a9b8485ae35b5555d9f5d20fc93a6cb6|8510706|https://github.com/astral-sh/ruff/releases/download/v0.4.9/ruff-0.4.9-x86_64-apple-darwin.tar.gz",
+        "0.4.9|linux_arm64|00c50563f9921a141ddd4ec0371149f3bbfa0369d9d238a143bcc3a932363785|8106747|https://github.com/astral-sh/ruff/releases/download/v0.4.9/ruff-0.4.9-aarch64-unknown-linux-musl.tar.gz",
+        "0.4.9|linux_x86_64|5ceba21dad91e3fa05056ca62f278b0178516cfad8dbf08cf2433c6f1eeb92d3|8863118|https://github.com/astral-sh/ruff/releases/download/v0.4.9/ruff-0.4.9-x86_64-unknown-linux-musl.tar.gz",


So in future, upgrading Ruff means updating these definitions right?

Yeah, following https://www.pantsbuild.org/2.23/docs/contributions/development/maintenance-tasks-and-scripts#external-tools-downloaded-executables, for instance.

I'd be imagining we append new versions here, rather than replace, so that someone who's pinned to (for example) 0.4.9 in Pants 2.23 via version = "0.4.9" can upgrade to Pants 2.24, 2.25, ... without having to change their Ruff config at all.

For instance, how terraform's maintained: #20469 (although maybe including all patch releases is a bit unnecessary for ruff).

Yes, we append!

krishnan-chandra · 2024-07-31T11:21:32Z

src/python/pants/backend/python/lint/ruff/subsystem.py

+        removal_version="2.26.0.dev0",
+        removal_hint="Now ignored: use `version` and `known_version` to customise the version, consider deleting the resolve and `python_requirement` if no longer used",


Since we are ignoring this option anyways, do we need 2 releases to deprecate it? It might be better to just force users into changing their options once in 2.23/2.24 so they don't have a false sense of security.

I guess I have the same question for the options below this as well

Just so we're talking about the same thing: are you questioning whether we include these at all, or questioning using 2.26.0.dev0 as the removal version?

questioning using 2.26.0.dev0 as the removal version?

This one.

I understand why it's included as it provides a smoother upgrade path for the user. But given that the old resolve-based functionality will no longer work, I don't see the point in leaving this in for 2 releases as the options are ignored. I think it makes sense to give 2 releases for deprecation when the old functionality still works to allow users to migrate. But since this is a breaking change, IMO we should force users to migrate to the new options ASAP.

Ah okay. I see where you're coming from.

I generally prefer longer deprecations: if we remove the options, it's harder for someone to do pre-release testing, and making prerelease testing as easy as possible is valuable IMO.

(Spelling out an example: if we removed the flags in 2.24, someone might be on 2.22, and then cannot do pre-release testing of any features of 2.24.0.devX releases (even non-ruff ones) until they fix the [ruff] part of pants.toml.)

That said, having the fields around in 2.25 in addition to 2.23 and 2.24 might be somewhat unnecessary, although it gives us a bit more flexibility/slack in terms of which release branches are live/stable when.

From my perspective forcing by removing the options seems to have less value:

if someone upgrades Pants and has Ruff's behaviour change due to the change in version, hopefully they'd notice that because CI fails (or whatever), and that'd prompt them to pay attention to the warning, hopefully.

if someone upgrades Pants and the behaviour does NOT change, that seems acceptable, and the user can notice and deal with the warning on their own time.

(Plus, the diagnostics being bad for an option that's fully removed, i.e. it's harder for someone to update in that case.)

I'll iterate on the message a bit:

$ MODE=debug PANTS_SOURCE=~/projects/pantsbuild/pants pants lint --only=ruff-check :: ... 12:57:53.58 [WARN] DEPRECATED: option 'install_from_resolve' in scope 'ruff' is scheduled to be removed in version 2.26.0.dev0. NOW IGNORED: use `version` and `known_version` options to customise the version of ruff, replacing this option; consider deleting the resolve and `python_requirement` if no longer used. See https://www.pantsbuild.org/2.23/reference/subsystems/ruff 12:57:54.51 [INFO] Completed: Lint with `ruff check` - ruff check succeeded. All checks passed! ...

Thoughts?

I buy it - I think pre-release testing is a valuable use case so we can leave it in.

tobni · 2024-07-31T15:12:56Z

A feature that my org quite enjoy is allowing a requirements.txt file to be the single source of truth for versions.

As background: Because pants pex processes are way too slow, local development takes place with a fat editable install venv (no pants) that install from requirements.txt.

CI does run pants.

Installing from resolve has the benefit that if I pin the version in a requirements.txt I can be sure the same version is used in ci and vice versa. With this change and the above mentioned setup, you need to set it in 2 places.

I'm not arguing that venvpex is the better approach, just wanted to air some install-from-resolve qualities that are nice.

huonw · 2024-08-01T00:05:14Z

Thanks for the input!

As background: Because pants pex processes are way too slow, local development takes place with a fat editable install venv (no pants) that install from requirements.txt.

Given this is eliminating the pex usage for ruff, pants lint --only=ruff :: might be fast again? Or am I misunderstanding what you mean?

huonw · 2024-08-01T02:56:29Z

Ah, hm, another question: if we could hypothetically pants export tools like this, would that solve the problem too?

lilatomic · 2024-08-01T03:11:39Z

Ah, hm, another question: if we could hypothetically pants export tools like this, would that solve the problem too?

I've thought about implementing this, and it shouldn't be that hard to implement on the backend. The question would be porcelain ones, like whether we'd want to make "resolves" for them (pants export --resolve=ruff) or something else (pants export --tool=ruff, or something). If that would help, I can open an issue and get to work.

tobni · 2024-08-01T08:08:06Z

So my colleagues do not use pants day to day, only the CI I built does, because pants is way too slow and resource intensive (our repo is medium sized, shy of 1 million lines of python code) for fast local dev iteration compared to a regular venv + editable install of the entire monorepo. Granted we do not use REAPI either.

With that in mind, asking them to run pants lint --only=ruff :: instead of ruff via the venv just looks like extra steps and OH for no gain.

pants export looks like the better approach to me because that I can incorporate in our dev-install scripts, and the devs dont have to invoke pants and pants.toml can contain the de-facto version.

huonw · 2024-08-01T23:31:52Z

Cool, sounds like a strong reason to support exporting such tools. I've filed #21251

huonw · 2024-09-11T21:33:32Z

We've just branched for 2.23, so merging this pull request now will come out in 2.24, please move the release notes updates to docs/notes/2.24.x.md. Thank you!

krishnan-chandra · 2024-10-22T00:56:44Z

Now that #21251 is merged, would love to get this in as well; @huonw if you don't have time, I'm happy to see this through and submit a PR with the change.

…nal-tool

huonw · 2024-10-24T05:26:31Z

Thanks for the prompt, I've updated this.

krishnan-chandra

Looks good!

krishnan-chandra · 2024-10-24T16:56:28Z

src/python/pants/backend/python/lint/ruff/rules_integration_test.py

-    assert lint_result.exit_code == 1
+    assert lint_result.exit_code == 0


Interesting. Since the test is intended to check that the files get skipped (which it certainly looks like they do), I agree with you and think the new behavior is clearer.

krishnan-chandra · 2024-10-24T17:02:21Z

src/python/pants/backend/python/lint/ruff/subsystem.py

-    default_lockfile_resource = ("pants.backend.python.lint.ruff", "ruff.lock")
+    help = "The Ruff Python formatter (https://github.com/astral-sh/ruff)."
+
+    default_version = "0.6.4"


Note - latest is now 0.7.1 but I think we can upgrade that in a follow-up rather than adding to this PR.

Yeah, agreed

benjyw

Great!

benjyw · 2024-10-30T03:54:22Z

src/python/pants/backend/python/lint/ruff/subsystem.py

+        "0.4.9|macos_arm64|5f4506d7ec2ae6ac5a48ba309218a4b825a00d4cad9967b7bbcec1724ef04930|8148128|https://github.com/astral-sh/ruff/releases/download/v0.4.9/ruff-0.4.9-aarch64-apple-darwin.tar.gz",
+        "0.4.9|macos_x86_64|e4d745adb0f5a0b08f2c9ca71e57f451a9b8485ae35b5555d9f5d20fc93a6cb6|8510706|https://github.com/astral-sh/ruff/releases/download/v0.4.9/ruff-0.4.9-x86_64-apple-darwin.tar.gz",
+        "0.4.9|linux_arm64|00c50563f9921a141ddd4ec0371149f3bbfa0369d9d238a143bcc3a932363785|8106747|https://github.com/astral-sh/ruff/releases/download/v0.4.9/ruff-0.4.9-aarch64-unknown-linux-musl.tar.gz",
+        "0.4.9|linux_x86_64|5ceba21dad91e3fa05056ca62f278b0178516cfad8dbf08cf2433c6f1eeb92d3|8863118|https://github.com/astral-sh/ruff/releases/download/v0.4.9/ruff-0.4.9-x86_64-unknown-linux-musl.tar.gz",


Yes, we append!

huonw added 3 commits July 31, 2024 12:38

Install and run ruff as an external tool, not python tool

b29b9aa

Restore options for smoother upgrade experience

7f98f99

Release notes

7aa09ea

huonw added the category:user api change label Jul 31, 2024

huonw commented Jul 31, 2024

View reviewed changes

huonw mentioned this pull request Jul 31, 2024

Update ruff to version 0.5.5 #21235

Closed

krishnan-chandra reviewed Jul 31, 2024

View reviewed changes

Pull out removal hint string

92e3f5a

Better message

0f9812d

huonw mentioned this pull request Aug 1, 2024

Support exporting downloaded external tools #21251

Closed

huonw added 5 commits October 24, 2024 12:37

Merge remote-tracking branch 'upstream/main' into huonw/ruff-as-exter…

955965a

…nal-tool

Adjust for new URLs

d888403

Add to 0.6.4 to align to main, leave 0.4.9 for 2.23 compat

78025c9

Adjust for new artifact contents

2f1e827

Move release notes

9e94396

huonw force-pushed the huonw/ruff-as-external-tool branch from 85bdf0d to 9e94396 Compare October 24, 2024 02:54

rm unused imports

ec3030f

huonw marked this pull request as ready for review October 24, 2024 05:27

huonw requested a review from krishnan-chandra October 24, 2024 05:27

huonw requested review from lilatomic and tobni October 24, 2024 05:28

krishnan-chandra approved these changes Oct 24, 2024

View reviewed changes

huonw requested a review from AlexTereshenkov October 27, 2024 22:34

benjyw approved these changes Oct 30, 2024

View reviewed changes

huonw merged commit f081583 into main Oct 30, 2024
25 checks passed

huonw deleted the huonw/ruff-as-external-tool branch October 30, 2024 04:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run ruff as an external tool, not via Python/PEX #21237

Run ruff as an external tool, not via Python/PEX #21237

huonw commented Jul 31, 2024 •

edited

Loading

huonw Jul 31, 2024

huonw Oct 24, 2024 •

edited

Loading

krishnan-chandra Oct 24, 2024

huonw commented Jul 31, 2024

krishnan-chandra Jul 31, 2024

huonw Jul 31, 2024

benjyw Oct 30, 2024

krishnan-chandra Jul 31, 2024

krishnan-chandra Jul 31, 2024

huonw Aug 1, 2024

krishnan-chandra Aug 1, 2024

huonw Aug 1, 2024

krishnan-chandra Aug 1, 2024

tobni commented Jul 31, 2024 •

edited

Loading

huonw commented Aug 1, 2024

huonw commented Aug 1, 2024

lilatomic commented Aug 1, 2024

tobni commented Aug 1, 2024 •

edited

Loading

huonw commented Aug 1, 2024

huonw commented Sep 11, 2024

krishnan-chandra commented Oct 22, 2024

huonw commented Oct 24, 2024

krishnan-chandra left a comment

krishnan-chandra Oct 24, 2024

krishnan-chandra Oct 24, 2024

huonw Oct 24, 2024

huonw Nov 2, 2024

benjyw left a comment

benjyw Oct 30, 2024

		assert lint_result.exit_code == 1
		assert lint_result.exit_code == 0

		removal_version="2.26.0.dev0",
		removal_hint="Now ignored: use `version` and `known_version` to customise the version, consider deleting the resolve and `python_requirement` if no longer used",

Run ruff as an external tool, not via Python/PEX #21237

Run ruff as an external tool, not via Python/PEX #21237

Conversation

huonw commented Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

huonw Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw commented Jul 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobni commented Jul 31, 2024 • edited Loading

huonw commented Aug 1, 2024

huonw commented Aug 1, 2024

lilatomic commented Aug 1, 2024

tobni commented Aug 1, 2024 • edited Loading

huonw commented Aug 1, 2024

huonw commented Sep 11, 2024

krishnan-chandra commented Oct 22, 2024

huonw commented Oct 24, 2024

krishnan-chandra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjyw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw commented Jul 31, 2024 •

edited

Loading

huonw Oct 24, 2024 •

edited

Loading

tobni commented Jul 31, 2024 •

edited

Loading

tobni commented Aug 1, 2024 •

edited

Loading