https://pantsbuild.org/ logo
#development
Title
# development
f

fast-nail-55400

02/23/2022, 6:48 PM
rule graph question. I have a synchronous invocation of the engine (for BSP) that is adding a “context” type as a second
subject
for the engine execution request. I am getting this error:
Copy code
ValueError: Encountered 4 rule graph errors:
  No installed rules return the type BSPContext, and it was not provided by potential callers of @rule(pants.backend.scala.bsp.rules:67:bsp_resolve_all_scala_build_targets(ScalaBSPBuildTargetsRequest, AllScalaTargets, BSPContext) -> BSPBuildTargets, gets=[Get(BuildTarget, ResolveScalaBSPBuildTargetRequest)]).
And adding
QueryRule(BSPContext, ())
did not help (same error). Thoughts?
cc @witty-crayon-22786
w

witty-crayon-22786

02/23/2022, 7:51 PM
the queryrule matches the callsite
so
scheduler.request(A, [Params(B, C)])
is
QueryRule(A, [B, C])
f

fast-nail-55400

02/23/2022, 7:52 PM
huh so it would have to be added to every
QueryRule
registered for BSP handler mappings?
w

witty-crayon-22786

02/23/2022, 7:52 PM
QueryRule(BSPContext, ())
is “BSPContext must be a singleton which can be computed with no arguments”
correct
er
i don’t know… if you want it to be…?
however you can get the callsite and declaration to line up.
for `@goal_rule`s in general, we supply a standard set of
Params
at the top… could do the same for handlers
f

fast-nail-55400

02/23/2022, 7:54 PM
ah that is why
goal_params_types
is there then
w

witty-crayon-22786

02/23/2022, 7:54 PM
if you made the handler API more complex, you could differentiate the queries/requests for different handlers
yea
this is because we do a subset match when kicking off a query: only params which are actually consumed end up in the key for the rule, but you can specify more than are needed to actually run the query
(callsite and query still need to be aligned: it’s just the compiled version that might be a subset)
f

fast-nail-55400

02/23/2022, 7:58 PM
I’m still getting the error. https://github.com/pantsbuild/pants/pull/14583
w

witty-crayon-22786

02/23/2022, 7:59 PM
note:
Copy code
scheduler.request(A, [Params(B, C)])
passing multiple subjects is not what you want here: you want a single subject with multiple params
f

fast-nail-55400

02/23/2022, 8:00 PM
so subject != param ?
w

witty-crayon-22786

02/23/2022, 8:00 PM
no: it’s a batch API, so each subject is a root in the request
sorry… it’s awkward. the batchness is not used in practice. RuleRunner wraps it and exposes a different API which is not batched, which is definitely easier to use
f

fast-nail-55400

02/23/2022, 8:02 PM
still fails even which I switch to:
Copy code
execution_request = self._scheduler_session.execution_request(
            products=[method_mapping.response_type], subjects=[Params(request, self._client_context)]
        )
for
pants.engine.internals.selectors.Params
w

witty-crayon-22786

02/23/2022, 8:02 PM
with which error
f

fast-nail-55400

02/23/2022, 8:03 PM
Copy code
No installed rules return the type BSPContext, and it was not provided by potential callers of @rule(pants.backend.scala.bsp.rules:67:bsp_resolve_all_scala_build_targets(ScalaBSPBuildTargetsRequest, AllScalaTargets, BSPContext) -> BSPBuildTargets, gets=[Get(BuildTarget, ResolveScalaBSPBuildTargetRequest)]).
    If that type should be computed by a rule, ensure that that rule is installed.
    If it should be provided by a caller, ensure that it is included in any relevant Query or Get.
  No installed rules return the type ScalaBSPBuildTargetsRequest, and it was not provided by potential callers of @rule(pants.backend.scala.bsp.rules:67:bsp_resolve_all_scala_build_targets(ScalaBSPBuildTargetsRequest, AllScalaTargets, BSPContext) -> BSPBuildTargets, gets=[Get(BuildTarget, ResolveScalaBSPBuildTargetRequest)]).
    If that type should be computed by a rule, ensure that that rule is installed.
    If it should be provided by a caller, ensure that it is included in any relevant Query or Get.
  No source of dependency AllScalaTargets for @rule(pants.backend.scala.bsp.rules:67:bsp_resolve_all_scala_build_targets(ScalaBSPBuildTargetsRequest, AllScalaTargets, BSPContext) -> BSPBuildTargets, gets=[Get(BuildTarget, ResolveScalaBSPBuildTargetRequest)]). All potential sources were eliminated: []
  No source of dependency Get(BuildTarget, ResolveScalaBSPBuildTargetRequest) for @rule(pants.backend.scala.bsp.rules:67:bsp_resolve_all_scala_build_targets(ScalaBSPBuildTargetsRequest, AllScalaTargets, BSPContext) -> BSPBuildTargets, gets=[Get(BuildTarget, ResolveScalaBSPBuildTargetRequest)]). All potential sources were eliminated: []
w

witty-crayon-22786

02/23/2022, 8:03 PM
that’s a graph compile error, so you’re not making it to the callsite.
is this in a test?
and is the Query definitely installed in that test?
f

fast-nail-55400

02/23/2022, 8:05 PM
no rule graph error if I remove the
BSPContext
as a parameter to the rule where it was added in the PR linked above
w

witty-crayon-22786

02/23/2022, 8:05 PM
or: do you have other queries installed in that test which now need to provide that param as well?
f

fast-nail-55400

02/23/2022, 8:05 PM
This is when I try to run
./pants fmt
on this PR
doesn’t even get close to a test
Pants won’t startup
w

witty-crayon-22786

02/23/2022, 8:07 PM
ok, pausing for a second. https://github.com/pantsbuild/pants/pull/14583 shows no
@rules
actually consuming the
BSPContext
yet, right?
nevermind, i see it.
if you remove the use of the
BSPContext
in
bsp_resolve_all_scala_build_targets
then things work, yea?
f

fast-nail-55400

02/23/2022, 8:09 PM
yes
w

witty-crayon-22786

02/23/2022, 8:09 PM
it’s ok for
@rules
to use less than the full query. so would start by getting things green with the new QueryRule and callsite before editing anything to consume it.
do you maybe have another
QueryRule
installed in a backend somewhere without the new param?
f

fast-nail-55400

02/23/2022, 8:11 PM
nope those got moved to engine_initializer.py
and the PR updates the QueryRule’s to be:
Copy code
*(
                    QueryRule(impl.response_type, (impl.request_type, BSPContext))
                    for impl in union_membership.get(BSPHandlerMapping)
                ),
w

witty-crayon-22786

02/23/2022, 8:12 PM
hm. let me take a look.
👍 1
ah. so: this is unfortunately probably related to https://github.com/pantsbuild/pants/issues/12934 … currently, when you use a union in a
Get
, we limit what can be consumed by the
Get
to exactly the specified type and nothing else.
UnionRule(BSPBuildTargetsRequest, ScalaBSPBuildTargetsRequest),
is gaining a dependency on the
BSPContext
, which will not be provided by that
Get
… and because of https://github.com/pantsbuild/pants/issues/12934 we have no way to declare additional parameters which should be provided to the implementation
f

fast-nail-55400

02/23/2022, 8:23 PM
would it make sense for
BSPContext
to be a session value?
(assuming that is a way to work around this issue with union gets)
w

witty-crayon-22786

02/23/2022, 8:25 PM
that, or expand the request type of
ScalaBSPBuildTargetsRequest
to have a
BSPContext
field
f

fast-nail-55400

02/23/2022, 8:26 PM
can session values be replaced in an existing scheduler?
but then I’d have to use
.new_session
and provide the session values like
UnionMembership
as well
w

witty-crayon-22786

02/23/2022, 8:27 PM
*session
UnionMembership is a singleton rather than a session value
can session values be replaced in an existing scheduler?
if not, a method for that would be fine
f

fast-nail-55400

02/23/2022, 8:28 PM
never mind, meant this stanza:
Copy code
session_values=SessionValues(
                {
                    OptionsBootstrapper: options_bootstrapper,
                    CompleteEnvironment: env,
                }
            ),
w

witty-crayon-22786

02/23/2022, 8:31 PM
could add a rust method to add a value to the session values
sorry for the trouble here. i have made further progress on the rule graph changes since then, but https://github.com/pantsbuild/pants/pull/12966 took a lot of time pressure off
f

fast-nail-55400

02/23/2022, 8:34 PM
interesting.
and if I make
BSPContext
mutable (to allow updating it when the client connects and then remains immutable), will that play nicely with the engine?
i.e., it would be mutated in the BSP protocol driver after the dispatch to the engine of the `build/initialize`BSP method completes
w

witty-crayon-22786

02/23/2022, 8:37 PM
it makes me squeamish, but… it would work, yea. it would definitely be better for the state to be stored elsewhere, but.
f

fast-nail-55400

02/23/2022, 8:38 PM
I’d rather pass it in via a Params as a subject, but that does not seem like it is feasible currently.
w

witty-crayon-22786

02/23/2022, 8:38 PM
if you can avoid doing that, and have the state stored externally, that would definitely be better, because i’d like to replace usage of SessionValues at some point with making the RuleGraph act the way you’d expect for the original case you had here
f

fast-nail-55400

02/23/2022, 8:38 PM
And adding it as a field on the request in question seems clunky
w

witty-crayon-22786

02/23/2022, 8:38 PM
but if this ends up being the last consumer of SessionValues, that’s ok too.
f

fast-nail-55400

02/23/2022, 8:39 PM
I could make it be a global singleton, but that goes contrary to the spirit in which engine rules are supposed to be written
i.e., pure functions
will try session values approach for now
w

witty-crayon-22786

02/23/2022, 8:44 PM
yea, fine with me. it’s a relatively cheap mechanism to keep around, even if everything else migrates away