https://pantsbuild.org/ logo
Powered by
h

hundreds-breakfast-49010

02/14/2020, 2:45 AM
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

hundreds-father-404

02/14/2020, 2:52 AM
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

witty-crayon-22786

02/14/2020, 4:31 AM
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-breakfast-49010

02/14/2020, 7:03 PM
@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

hundreds-father-404

02/14/2020, 7:03 PM
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

hundreds-breakfast-49010

02/14/2020, 7:04 PM
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

hundreds-father-404

02/14/2020, 7:06 PM
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

hundreds-breakfast-49010

02/14/2020, 7:08 PM
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

hundreds-father-404

02/14/2020, 7:15 PM
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

hundreds-breakfast-49010

02/14/2020, 7:18 PM
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

witty-crayon-22786

02/14/2020, 7:41 PM
and look at what you need for 
Select(Repl)
^ concretely this.
(this probably shouldn't be the answer, to be clear)
h

hundreds-father-404

02/14/2020, 7:43 PM
./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

witty-crayon-22786

02/14/2020, 7:43 PM
that error message should render all 
Select(Repl)
roots
h

hundreds-father-404

02/14/2020, 7:43 PM
I then run 
./viz/viz.py viz/rule_graph.dot
w

witty-crayon-22786

02/14/2020, 7:44 PM
@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

hundreds-breakfast-49010

02/14/2020, 7:49 PM
right
w

witty-crayon-22786

02/14/2020, 7:49 PM
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

hundreds-breakfast-49010

02/14/2020, 7:50 PM
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

witty-crayon-22786

02/14/2020, 7:51 PM
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

hundreds-breakfast-49010

02/14/2020, 7:52 PM
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

witty-crayon-22786

02/14/2020, 7:52 PM
so... not your fault... we should improve that error message.
h

hundreds-breakfast-49010

02/14/2020, 7:53 PM
well I don't know if it will work if I do mkae sure to add that rule
w

witty-crayon-22786

02/14/2020, 7:53 PM
basically, if there are no ways to compute some type, then ... you haven't installed the rule that computes it.
h

hundreds-breakfast-49010

02/14/2020, 7:53 PM
ah okay tests passed
💯 1
w

witty-crayon-22786

02/14/2020, 7:53 PM
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

hundreds-breakfast-49010

02/14/2020, 7:54 PM
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

witty-crayon-22786

02/14/2020, 7:54 PM
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

hundreds-breakfast-49010

02/14/2020, 7:55 PM
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

witty-crayon-22786

02/14/2020, 7:56 PM
yep.
because otherwise the RuleGraph would not have successfully compiled.
h

hundreds-breakfast-49010

02/14/2020, 7:56 PM
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

witty-crayon-22786

02/14/2020, 7:58 PM
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

hundreds-breakfast-49010

02/14/2020, 8:08 PM
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

witty-crayon-22786

02/14/2020, 8:10 PM
yep, will do.
h

hundreds-breakfast-49010

02/14/2020, 9:03 PM
Powered by