https://pantsbuild.org/ logo
Powered by
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
Powered by