https://pantsbuild.org/ logo
Join Slack
Powered by
rule graph question. I have a synchronous invocati...
# development
f
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
the queryrule matches the callsite
so 
scheduler.request(A, [Params(B, C)])
is 
QueryRule(A, [B, C])
f
huh so it would have to be added to every 
QueryRule
registered for BSP handler mappings?
w
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
ah that is why 
goal_params_types
is there then
w
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
I’m still getting the error. https://github.com/pantsbuild/pants/pull/14583
w
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
so subject != param ?
w
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
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
with which error
f
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
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
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
or: do you have other queries installed in that test which now need to provide that param as well?
f
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
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
yes
w
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
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
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
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
that, or expand the request type of 
ScalaBSPBuildTargetsRequest
to have a 
BSPContext
field
f
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
*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
never mind, meant this stanza:
Copy code
session_values=SessionValues(
                {
                    OptionsBootstrapper: options_bootstrapper,
                    CompleteEnvironment: env,
                }
            ),
w
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
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
it makes me squeamish, but… it would work, yea. it would definitely be better for the state to be stored elsewhere, but.
f
I’d rather pass it in via a Params as a subject, but that does not seem like it is feasible currently.
w
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
And adding it as a field on the request in question seems clunky
w
but if this ends up being the last consumer of SessionValues, that’s ok too.
f
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
yea, fine with me. it’s a relatively cheap mechanism to keep around, even if everything else migrates away
Open in Slack
PreviousNext
Powered by