https://pantsbuild.org/ logo
b

bitter-ability-32190

10/04/2022, 6:07 PM
Need some 🧠 on a change I'm working on 🧵
💭 1
I'm changing
lint
and
fmt
to move the result streaming to be an implementation detail of the core goal, instead of making users pass in information that the core goal knows solely for streaming. There's tradeoffs, but I really like the result in terms of plugin authors needing less boilerplate. And less room for error. However there is one giant massive blocker.
LintResult
needs an
exit_code
which for formatters we use
1 if changed else 0
. the new
LintResult
and
FmtResult
won't know if they changed because they won't have the
input
re-plumbed through. So converting from a
FmtResult
to a
LintResult
is naturally impossible 😐 There is a possible solution of completely abusing the type of
exit_code
though. technically we can just set
exit_code = output_snapshot
and then
lint.py
has a
input_snpashot: Snapshot = request.exit_code
(if formatter). It works but YIKES! Thoughts?
CC @hundreds-father-404 who always has great ideas ❤️
w

witty-crayon-22786

10/04/2022, 7:13 PM
I’m changing
lint
and
fmt
to move the result streaming to be an implementation detail of the core goal, instead of making users pass in information that the core goal knows solely for streaming.
i’m not sure i understand what you mean here, so i’m not understanding the tradeoffs in the rest of the question
b

bitter-ability-32190

10/04/2022, 7:15 PM
Copy code
@dataclass(frozen=True)
-class LintResult(EngineAwareReturnType):
+class LintResult:
     exit_code: int
     stdout: str
     stderr: str
-    linter_name: str
-    partition_description: str | None = None
     report: Digest = EMPTY_DIGEST
(so plugin authors no longer pipe these through) and then for that to work:
Copy code
+
+@dataclass(frozen=True)
+class _StreamedLintRequest:
+    subpartition: LintRequest.SubPartition
+    linter_name: str
+    is_formatter: bool
+
+
+@dataclass(frozen=True)
+class _StreamedLintResult(EngineAwareReturnType):
+    result: LintResult
+    linter_name: str
+    is_formatter: bool
+    partition_description: str | None = None
Then we:
Copy code
Get(
+            _StreamedLintResult,
+            _StreamedLintRequest(
and then:
Copy code
+@rule(desc="Lint", level=LogLevel.DEBUG)
+async def stream_lint_result(request: _StreamedLintRequest) -> _StreamedLintResult:
+    result = await Get(LintResult, LintRequest.SubPartition, request.subpartition)
w

witty-crayon-22786

10/04/2022, 7:17 PM
got it.
b

bitter-ability-32190

10/04/2022, 7:17 PM
The change is mroe drastic for
fmt
, where the
input
Snapshot is piped through
It's really an optional change, but I like the simpler result type
w

witty-crayon-22786

10/04/2022, 7:18 PM
uh, so honestly: i think it’s a bit of a bad sign in terms of how
EngineAwareReturnType
works if we end up needing to do this to make things easier for plugin authors. it might be a sign that making this return type driven rather than callsite driven is a mistake, for example.
(particularly if your concern generalizes to other uses of
EngineAwareReturnType
)
b

bitter-ability-32190

10/04/2022, 7:18 PM
Yeah, I suspected that might be an answer as well 😛 Which I'm OK with 🙂
The
name
aspect can be done via customized classtypes with the
name
as a classvar. The
input
snapshot, not so much 😛
(oh wait nevermind, we can't "demote" return types like we can input types+unions)
w

witty-crayon-22786

10/04/2022, 7:20 PM
it’s a question of how awkward it is, i think. callsite driven syntax might look like:
Copy code
await Get(LintResult, Input(..), log_with=lambda x: ...)
🤔 1
b

bitter-ability-32190

10/04/2022, 7:20 PM
I like this direction of thinking 🙂
w

witty-crayon-22786

10/04/2022, 7:21 PM
(just as a thought experiment… maybe it’s only on
MultiGet
instead or something)
on the flip side, i am hoping to expand usage of `EngineAwareParam`… and
EngineAwareReturnType
driving cacheability continues to make sense. it’s just possible that the fact that we’re using
EngineAwareReturnType
to trigger side-effects for streaming might not be the best.
1
(unclear… it’s pretty minimal, and hard to get wrong.)
b

bitter-ability-32190

10/04/2022, 7:28 PM
This was going to be my solution to the streaming issue on 2.15 w.r.t. formatters double-streaming. So anything we can decide/whip up in 2.15 would be ideal 😛
I think we can hack a workaround though if we hit the buzzer
w

witty-crayon-22786

10/04/2022, 7:31 PM
This was going to be my solution to the streaming issue on 2.15 w.r.t. formatters double-streaming.
is that one filed anywhere? i don’t remember what the issue was
b

bitter-ability-32190

10/04/2022, 7:32 PM
Just in my noggin. https://github.com/pantsbuild/pants/blob/7acbcda329db6fb9a2ebcd0a547c3e0ab123ba0c/src/python/pants/core/goals/fmt.py#L323 copnverts a streamed result to a streamed result. So double-stream
👍 1
I know this doesn't quite map out in terms of rule engine, per-se. But in a high-level sense you kinda want the return type streaming levers/knobs to also take in the
request
type that is the input (I know a rule has multiple inputs which is why this is funky)
Maybe it can just accept all the inputs? Then the author picks-and-chooses?
w

witty-crayon-22786

10/04/2022, 8:52 PM
you mean have the implementation of
EngineAwareReturnType
take as an argument the params that were in scope?
that would be feasible, yea.
it’s … pretty magical, and untyped. but yea, would work.
b

bitter-ability-32190

10/04/2022, 8:54 PM
Thoughts on that?
w

witty-crayon-22786

10/04/2022, 8:57 PM
it would be very, very freefrom, in the sense that if you used the presence of a param to decide to do something, and then that param was in scope in some other use case, you’d have no way to prevent triggering the message twice
and “which params are in scope” is a very nebulous concept: it’s not the positional arguments, per-se. if it were the positional arguments it might be more stable. but you still wouldn’t have context on which
@rule
the
EngineAwareReturnType
was returned from
to make this more concrete: maybe experiment with the code and see if you can find a hack you like. the API is consumed here: https://github.com/pantsbuild/pants/blob/d882bd6755a10446875a882700e5824110d1d228/src/rust/engine/src/nodes.rs#L1194-L1198
b

bitter-ability-32190

10/04/2022, 9:00 PM
Yeah ok thinking about it more. I don't like this idea. Bummer 🙂
w

witty-crayon-22786

10/04/2022, 9:00 PM
i.e.
engine_aware.py
is consumed by
<http://engine_aware.rs|engine_aware.rs>
b

bitter-ability-32190

10/06/2022, 5:25 PM
I suppose a really easy "solution" currently is to have the request type hold the result-type constructor:
request.create_result(....)
I'll likely go forward with this as the unblocker of nicer plugin code