https://pantsbuild.org/ logo
Join Slack
Powered by
I've been having a really frustrating time writing...
# development
h
I've been having a really frustrating time writing tests for the various iterations of the v2 repl goal, and I'm not entirely sure why
my specific issue right now is that, trying to move the repl logic from the python backend to a core rule in https://github.com/pantsbuild/pants/pull/9077/commits/e4bc6d515799b2570cdf78ffadfaaa33a4c7a0f4 , I'm missing some rule that's making the unit test no longer pass even though it passed when I had the former deisgn
but it took a long time to figure out the right rule setup for that test as well
and in general it feels like it takes a lot of blind trial and error to figure out which subset of the installed pants rules you need to register to use the 
goal_rule_test_base
framework to test a new goal
which is less than ideal
h
One improvement I think we should make that you and I DMed about is to start having rule files register every direct rule dependency in their 
rules()
function. If a rule in the file directly uses 
PythonSetup
, for example, then register 
subsystem_rule(PythonSetup)
, even if that was registered by a transitive rule so it’s not strictly necessary. The engine will deduplicate the rules This is similar to BUILD file hygiene. It’s best practice to declare every direct dependency, even if you don’t need to because a transitive dependency already includes it The impact of this would be that tests only need to register the 
rules()
for the file they are directly testing and no longer need to register all the transitive rules used, like they currently do. Should make test setup much simpler
w
sorry for the trouble. my hope is that we can improve rule graph error messages to the point where it isn't necessary to couple packages like that.
h
@hundreds-father-404 so that would imply a one-time manual audit + update of every 
rule()
function, and then making sure that we keep that in sync going forward
h
Yes.
and then making sure that we keep that in sync going forward
Which isn’t wonderful. See how painful it is to keep in sync BUILD files
h
right anything that you have to manually keep in sync is prone to falling out of sync as soon as people stop paying attention
👍 1
I'm looking at the error 
E       Exception: No installed @rules can compute Repl for input Params((AddressSpecs, Console, Factory, InteractiveRunner, OptionsBootstrapper, Workspace)).
right now. I think what might be going on is that there's a bunch of rules around handling AddressSpecs in 
build_files.py
that I'm not including
but I'm not sure about that, that's just a guess
(I'm not including them because they involve creating a mock 
AddressMapper
which I don't really understand how to mock; but in any case this is pretty far away from the original problem I'm trying to solve)
h
Those should already be included iirc. We never needed to register 
build_files.py
in other tests, including ones that use 
FilesystemSpecs
and 
AddressesWithOrigins
h
that makes sense
I wonder if it would be helpful to add the 
param_inputs
into the error messages in 
find_root_edges
we should have some information about what specific rule lookup is failing there, right?
(this is 
find_root_edges
in 
rule_graph/src/lib.rs
)
we hit that error in that code when we hit the case 
0 if params.is_subset(&self.root_param_types) => {
so maybe what's going on is that something that's supposed to be a RootRule isn't registered as such in the test (and the error message might be able to suggest this?)
h
One thing to try. Add 
RootRule(PythonRepl)
Oh I see you have two two test classes. Which one are you testing? 
src/python/pants/backend/python/rules/repl_integration_test.py
should not use 
GoalRuleTestBase
because it’s not testing a goal. It should use 
TestBase
and 
self.request_single_product
h
I'm trying to move the code under the python backend into a similar test under rules/core
that will test the top-level goal rule
and eventually delete the python-specific one when this is working
anyway adding 
RootRule(PythonRepl)
does nothing
w
and look at what you need for 
Select(Repl)
^ concretely this.
(this probably shouldn't be the answer, to be clear)
h
./pants --native-engine-visualize-to=viz/
. I have a little script to then open it up in Chrome:
Copy code
#!/usr/bin/env python3

import argparse
import subprocess

def main() -> None:
  fp = create_parser().parse_args().fp
  subprocess.run(["dot", "-Tpdf", "-O", fp])
  subprocess.run(["open", "-a", "Google Chrome", f"{fp}.pdf"])


def create_parser() -> argparse.ArgumentParser:
  parser = argparse.ArgumentParser(description='Visualize graph and open as PDF.')
  parser.add_argument('fp', help='e.g. 000 or 013')
  return parser


if __name__ == '__main__':
  main()
w
that error message should render all 
Select(Repl)
roots
h
I then run 
./viz/viz.py viz/rule_graph.dot
w
@hundreds-father-404: see what i mean about the error message?
it looks like we already attempt to give suggestions
@hundreds-breakfast-49010: in the context of your specific error, we should try to understand why you are not getting useful suggestions there.
h
right
w
because what it should be doing is rendering all 
Select(Repl)
nodes.
which should tell you precisely what you can give it.
what is the complete error message?
h
the complete error message is 
Exception: No installed @rules can compute Repl for input Params((AddressSpecs, Console, Factory, InteractiveRunner, OptionsBootstrapper, Workspace)).
, with no suggestions
I can see that the code that produces that error is trying to produce suggestions butit has none in my case
w
and the rule that computes a 
Repl
is definitely installed...?
what should have happened here is that if that rule is installed, then there must be at least one way to compute a 
Repl
, otherwise the graph would have failed to be constructed even early
h
oh darnit I think that may have been my problem, I didn't move the right function named 
repl
becuase I was copying code raound in this commit
👍 1
w
so... not your fault... we should improve that error message.
h
well I don't know if it will work if I do mkae sure to add that rule
w
basically, if there are no ways to compute some type, then ... you haven't installed the rule that computes it.
h
ah okay tests passed
💯 1
w
if there are any ways to compute some type, they should be rendered as suggestions there.
i'll post an edit for that message.
❤️ 1
h
so yeah that was just me not realizing that I hadn't included a rule becuase it was named nearly identically to one I had included, in a contxt where I'm moving code around between multiple files
@witty-crayon-22786 what edit do you think would've made sense here?
w
basically, if there are no ways to compute some type, then ... you haven't installed the rule that computes it.
if there are any ways to compute some type, they should be rendered as suggestions there.
in cases where there are no suggestions, it's because you're missing the rule that computes the type completely.
so we can say that, heh.
(the benefits of a compiled rule graph!)
h
So we can guarantee that we will only hit that specific error message, if it is the case that there is no registered rule whose return value is the 
produc
?
w
yep.
because otherwise the RuleGraph would not have successfully compiled.
h
ok, then adding some kind of suggestion to that error like "Are you sure that there is a registered rule of the form `-> Repl`" might be helpful
👍 1
w
yep, exactly.
will do.
so... i think that this might be the edit:
Copy code
} else {
           suggestions.sort();
           format!(
-            ", but there were @rules that could compute it using:\n  {}",
+            " for input Params({}), but there were @rules that could compute it using:\n  {}",
             suggestions.join("\n  ")
           )
         };
         Err(format!(
-          "No installed @rules can compute {} for input Params({}){}",
+          "No installed @rules can compute {}{}",
           product,
           params_str(&params),
           suggestions_str,
basically, don't bother mentioning parameters when it has nothing to do with which parameters are there.
👍 2
so your error message in this case would be: 
No installed @rules can compute Repl.
i guess that rather than "compute", we could say: "return"
and adjust the language for the Params case.
h
I might add a line about "check to be sure that a rule that provides <type> is actually installed"
to make it more actionable
w
yep, will do.
h
Open in Slack
PreviousNext
Powered by