Remove per-language indirection for formatter plugins #14166

stuhood · 2022-01-13T22:39:56Z

To prepare to add batching for formatters as part of #13462, this change removes the need to implement per-backend @rules that pipeline FmtRequests that apply to a particular language. Instead, targets are grouped by which FmtRequests apply to them, and then those requests are run sequentially.

There will be further changes to the formatting API in support of #13462, so this API is not final.

[ci skip-rust]
[ci skip-build-wheels]

[ci skip-rust] [ci skip-build-wheels]

stuhood · 2022-01-13T22:50:05Z

src/python/pants/core/goals/fmt.py

+    # Group targets by the sequence of FmtRequests that apply to them.
+    targets_by_fmt_request_order = defaultdict(list)
+    for target in targets:
+        fmt_requests = []
+        for fmt_request in union_membership[FmtRequest]:
+            if fmt_request.field_set_type.is_applicable(target):  # type: ignore[misc]
+                fmt_requests.append(fmt_request)
+        targets_by_fmt_request_order[tuple(fmt_requests)].append(target)


This is the relevant bit of logic for the grouping.

Eric-Arellano

Woohoo!

This has the same problem we had before: things will error if the same target appears in two distinct fmt_request_orders because they won't be piped to each other. I think the solution (for now) is to simply validate and error if the same target appears in two distinct fmt_request_orders. Wdyt?

Eric-Arellano · 2022-01-13T23:04:17Z

src/python/pants/core/goals/style_request.py

+    # TODO: Move this onto `FmtRequest`.
    prior_formatter_result: Snapshot | None = None


stuhood · 2022-01-13T23:19:28Z

This has the same problem we had before: things will error if the same target appears in two distinct fmt_request_orders because they won't be piped to each other.

A target cannot appear in two different request orders: the outer loop is over targets, and the inner loop is over request types.

Eric-Arellano · 2022-01-13T23:22:14Z

A target cannot appear in two different request orders: the outer loop is over targets, and the inner loop is over request types.

Ah ha. Duh. Would you be willing to expand the comment please pointing that out? More generally, it might be helpful to make explicit in the comment why we have to use a for loop within each request order, but why it's safe to parallelize across request orders.

stuhood · 2022-01-13T23:23:34Z

Would you be willing to expand the comment please pointing that out?

Sure: will do in the followup.

Internal changes: * upgrade to Rust v1.58.0 ([pantsbuild#14174](pantsbuild#14174)) * [internal] fix typos in codegen registration ([pantsbuild#14172](pantsbuild#14172)) * Remove per-language indirection for formatter plugins. ([pantsbuild#14166](pantsbuild#14166)) * Pulls `Coordinate` and `ArtifactRequirements` out into a separate file to avoid a circuilar import later ([pantsbuild#14164](pantsbuild#14164)) * Factors lockfile metadata code into class that supports multiple languages as well as versions ([pantsbuild#14116](pantsbuild#14116)) * Adds `.env` file to make vscode auto-imports work correctly out of the box ([pantsbuild#14130](pantsbuild#14130)) * [internal] Rename classes for `generate_lockfiles.py` for clarity ([pantsbuild#14218](pantsbuild#14218)) * [internal] Fix bad Find+Replace ([pantsbuild#14213](pantsbuild#14213)) * [internal] Bump libc from 0.2.106 to 0.2.112 in /src/rust/engine ([pantsbuild#14206](pantsbuild#14206)) * [internal] Bump tempfile from 3.2.0 to 3.3.0 in /src/rust/engine ([pantsbuild#14202](pantsbuild#14202)) * [internal] Bump criterion from 0.3.3 to 0.3.5 in /src/rust/engine ([pantsbuild#14205](pantsbuild#14205)) * [internal] Bump walkdir from 2.3.1 to 2.3.2 in /src/rust/engine ([pantsbuild#14204](pantsbuild#14204)) * [internal] Bump generic-array from 0.14.4 to 0.14.5 in /src/rust/engine ([pantsbuild#14203](pantsbuild#14203)) * [internal] Remove the minimum bucket size of batching to improve stability. ([pantsbuild#14210](pantsbuild#14210)) * [internal] Make `JvmLockfileRequest` generic ([pantsbuild#14201](pantsbuild#14201)) * [internal] add comment re clippy issue ([pantsbuild#14188](pantsbuild#14188)) * [internal] rename some codegen scopes to put language first ([pantsbuild#14187](pantsbuild#14187)) * [internal] Check for ambiguity when running `generate-lockfiles` ([pantsbuild#14178](pantsbuild#14178)) * [internal] Change JVM lockfile format ([pantsbuild#14175](pantsbuild#14175)) * [internal] go: rewrite third party package analysis to not use `go list` ([pantsbuild#14157](pantsbuild#14157)) * [internal] Hotfix `fmt` crashing on non-formattable targets ([pantsbuild#14168](pantsbuild#14168)) * [internal] Simplify `core/goals/package.py` filesystem API usage ([pantsbuild#14162](pantsbuild#14162)) * [internal] java/thrift: register union that was not registered ([pantsbuild#14159](pantsbuild#14159)) * [internal] Bump arc-swap from 1.3.0 to 1.5.0 in /src/rust/engine ([pantsbuild#14148](pantsbuild#14148)) * [internals] Bump tokio-rustls from 0.22.0 to 0.23.2 in /src/rust/engine ([pantsbuild#14149](pantsbuild#14149)) * [internal] Bump num_cpus from 1.13.0 to 1.13.1 in /src/rust/engine ([pantsbuild#14150](pantsbuild#14150)) * [internal] Bump tokio-util from 0.6.7 to 0.6.9 in /src/rust/engine ([pantsbuild#14151](pantsbuild#14151)) * [internal] Bump os_pipe from 0.9.2 to 1.0.0 in /src/rust/engine ([pantsbuild#14152](pantsbuild#14152)) * [internal] Introduce new `BuiltinGoal` subsystem type. ([pantsbuild#13991](pantsbuild#13991)) * [internal] Upgrade python type stubs packages ([pantsbuild#14143](pantsbuild#14143)) * [internal] Bump strum_macros from 0.20.1 to 0.23.1 in /src/rust/engine ([pantsbuild#14141](pantsbuild#14141)) * [internal] Make `generate-lockfiles` goal generic ([pantsbuild#14122](pantsbuild#14122)) * [internal] Bump errno from 0.2.7 to 0.2.8 in /src/rust/engine ([pantsbuild#14137](pantsbuild#14137)) * [internal] Bump colored from 1.9.3 to 2.0.0 in /src/rust/engine ([pantsbuild#14138](pantsbuild#14138)) * [internal] Bump crossbeam-channel from 0.4.4 to 0.5.0 in /src/rust/engine ([pantsbuild#14139](pantsbuild#14139)) * [internal] Bump reqwest from 0.11.4 to 0.11.9 in /src/rust/engine ([pantsbuild#14135](pantsbuild#14135)) * [internal] Further tweak dependabot config ([pantsbuild#14132](pantsbuild#14132)) * [internal] python: use immutable_input_digests for protobuf codegen ([pantsbuild#13885](pantsbuild#13885)) * [internal] python: use immutable_input_digests for protobuf codegen ([pantsbuild#13885](pantsbuild#13885)) * [internal] No need for two f-strings/two strings. ([pantsbuild#14119](pantsbuild#14119))

Remove LanguageFmtRequest indirection.

7e080ff

[ci skip-rust] [ci skip-build-wheels]

stuhood requested review from kaos, tdyas and Eric-Arellano January 13, 2022 22:39

stuhood commented Jan 13, 2022

View reviewed changes

Eric-Arellano approved these changes Jan 13, 2022

View reviewed changes

stuhood enabled auto-merge (squash) January 13, 2022 23:20

stuhood changed the title ~~Remove per-language indirection for the implementation of formatter plugins~~ Remove per-language indirection for formatter plugins Jan 13, 2022

stuhood merged commit 18114b9 into pantsbuild:main Jan 13, 2022

stuhood deleted the stuhood/fmt-language-generic branch January 13, 2022 23:38

Eric-Arellano mentioned this pull request Jan 14, 2022

[internal] Hotfix fmt crashing on non-formattable targets #14168

Merged

illicitonion mentioned this pull request Jan 22, 2022

Prep for 2.10.0.dev2 #14236

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove per-language indirection for formatter plugins #14166

Remove per-language indirection for formatter plugins #14166

stuhood commented Jan 13, 2022 •

edited

Loading

stuhood Jan 13, 2022

Eric-Arellano left a comment

Eric-Arellano Jan 13, 2022

stuhood commented Jan 13, 2022

Eric-Arellano commented Jan 13, 2022

stuhood commented Jan 13, 2022

		# TODO: Move this onto `FmtRequest`.
		prior_formatter_result: Snapshot \| None = None

Remove per-language indirection for formatter plugins #14166

Remove per-language indirection for formatter plugins #14166

Conversation

stuhood commented Jan 13, 2022 • edited Loading

stuhood Jan 13, 2022

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano Jan 13, 2022

Choose a reason for hiding this comment

stuhood commented Jan 13, 2022

Eric-Arellano commented Jan 13, 2022

stuhood commented Jan 13, 2022

stuhood commented Jan 13, 2022 •

edited

Loading