pants icon indicating copy to clipboard operation
pants copied to clipboard

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

Open huonw opened this issue 1 year ago • 8 comments

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 = "..."
  • eliminate any Python/pex overhead by invoking the binary directly
  • side-step fiddle like interpreter constraints (https://github.com/pantsbuild/pants/pull/21235#issuecomment-2258933919)

Potential problems:

  • I think we lose the ability to export the tool to a venv, for running locally. Is that a major issue?
  • 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... https://github.com/astral-sh/ruff/issues/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 current version (0.4.9), 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.

Still to do:

  • add some more versions, e.g. 0.4.4 (the version in Pants 2.22), 0.4.10 (latest in the 0.4.x release series), 0.5.5 (latest in the 0.5.x release series, #21235)
  • flag this as a plugin api change, in case someone is using the ruff subsystem directly
  • work out why the exit code changes

huonw avatar Jul 31 '24 03:07 huonw

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?

huonw avatar Jul 31 '24 03:07 huonw

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.

tobni avatar Jul 31 '24 15:07 tobni

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 avatar Aug 01 '24 00:08 huonw

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

huonw avatar Aug 01 '24 02:08 huonw

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.

lilatomic avatar Aug 01 '24 03:08 lilatomic

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.

tobni avatar Aug 01 '24 08:08 tobni

Cool, sounds like a strong reason to support exporting such tools. I've filed https://github.com/pantsbuild/pants/issues/21251

huonw avatar Aug 01 '24 23:08 huonw

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!

huonw avatar Sep 11 '24 21:09 huonw

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.

krishnan-chandra avatar Oct 22 '24 00:10 krishnan-chandra

Thanks for the prompt, I've updated this.

huonw avatar Oct 24 '24 05:10 huonw