[engine] refine exception output

Review Request #3992 — Created June 10, 2016 and submitted

wisechengyi
pants
3535, 3584
pants-reviews
kwlzn, nhoward_tw, peiyu, stuhood
  1. Output exception details on leaf trace nodes as opposed to lots of repetitions as linked in the bug.
  2. newline Throw for clarity.

https://travis-ci.org/pantsbuild/pants/builds/138217773

[tw-mbp-yic pants (3535_b)]$ ./pants --enable-v2-engine dependees examples/tests/::
Exception caught: (<class 'pants.build_graph.address_lookup_error.AddressLookupError'>)
File "/Users/yic/workspace/pants/src/python/pants/bin/pants_exe.py", line 50, in <module>
main()
File "/Users/yic/workspace/pants/src/python/pants/bin/pants_exe.py", line 44, in main
PantsRunner(exiter).run()
File "/Users/yic/workspace/pants/src/python/pants/bin/pants_runner.py", line 57, in run
options_bootstrapper=options_bootstrapper)
File "/Users/yic/workspace/pants/src/python/pants/bin/pants_runner.py", line 46, in _run
return LocalPantsRunner(exiter, args, env, options_bootstrapper=options_bootstrapper).run()
File "/Users/yic/workspace/pants/src/python/pants/bin/local_pants_runner.py", line 53, in run
self._maybe_profiled(self._run)
File "/Users/yic/workspace/pants/src/python/pants/bin/local_pants_runner.py", line 50, in _maybe_profiled
runner()
File "/Users/yic/workspace/pants/src/python/pants/bin/local_pants_runner.py", line 93, in _run
self._exiter).setup()
File "/Users/yic/workspace/pants/src/python/pants/bin/goal_runner.py", line 84, in init
build_graph)
File "/Users/yic/workspace/pants/src/python/pants/bin/goal_runner.py", line 99, in _select_buildgraph
return graph_helper.create_graph(root_specs)
File "/Users/yic/workspace/pants/src/python/pants/bin/engine_initializer.py", line 70, in create_graph
for _ in graph.inject_specs_closure(spec_roots): # Ensure the entire generator is unrolled.
File "/Users/yic/workspace/pants/src/python/pants/engine/legacy/graph.py", line 171, in inject_specs_closure
for address in self._inject(specs):
File "/Users/yic/workspace/pants/src/python/pants/engine/legacy/graph.py", line 183, in _inject
self._index(request.roots)
File "/Users/yic/workspace/pants/src/python/pants/engine/legacy/graph.py", line 68, in _index
'Build graph construction failed for {}:\n{}'.format(node.subject, trace))

Exception message: Build graph construction failed for DescendantAddresses(directory='examples/tests'):
Computing LegacyTarget for DescendantAddresses(directory='examples/tests')
Computing LegacyTarget for examples/tests/scala/org/pantsbuild/example/hello/welcome:welcome
Computing LegacyTarget for examples/src/scala/org/pantsbuild/example/hello/welcome:welcome
Computing LegacyTarget for examples/src/java/org/pantsbuild/example/hello/greet:greet
Computing TargetAdaptor for examples/src/java/org/pantsbuild/example/hello/greet:greet
Computing Struct for examples/src/java/org/pantsbuild/example/hello/greet:greet
Computing UnhydratedStruct for examples/src/java/org/pantsbuild/example/hello/greet:greet
Computing AddressFamily for Dir(path=u'examples/src/java/org/pantsbuild/example/hello/greet')
Throw(exc=MappingError(u"Failed to parse examples/src/java/org/pantsbuild/example/hello/greet/BUILD:\nname 'abc' is not defined",))
Computing LegacyTarget for examples/tests/java/org/pantsbuild/example/hello/greet:greet
Throw(exc=MappingError(u"Failed to parse examples/src/java/org/pantsbuild/example/hello/greet/BUILD:\nname 'abc' is not defined",))

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
WI
  1. i guess it is possible not to load all traces into a list, but not sure if that's needed given pants should exit out pretty early if there is any error at graph building.

  2. 
      
KW
  1. wondering if this might be a good candidate for a unit test - if for nothing else to prove (and pin) the correctness for a more complex graph trace?

    1. this part can be a little tricky. I suppose the order to graph traversal is not the same every time, so the output may differ.

    2. examples without any build file change.

        Computing LegacyTarget for DescendantAddresses(directory='examples/tests')
          Computing LegacyTarget for examples/tests/scala/org/pantsbuild/example/hello/welcome:welcome
            Computing LegacyTarget for examples/src/scala/org/pantsbuild/example/hello/welcome:welcome
              Computing LegacyTarget for examples/src/java/org/pantsbuild/example/hello/greet:greet
                Computing TargetAdaptor for examples/src/java/org/pantsbuild/example/hello/greet:greet
                  Computing Struct for examples/src/java/org/pantsbuild/example/hello/greet:greet
                    Computing UnhydratedStruct for examples/src/java/org/pantsbuild/example/hello/greet:greet
                      Computing AddressFamily for Dir(path=u'examples/src/java/org/pantsbuild/example/hello/greet')
                        Throw(exc=MappingError(u"Failed to parse examples/src/java/org/pantsbuild/example/hello/greet/BUILD:\nname 'abc' is not defined",))
          Computing LegacyTarget for examples/tests/java/org/pantsbuild/example/hello/greet:greet
            Throw(exc=MappingError(u"Failed to parse examples/src/java/org/pantsbuild/example/hello/greet/BUILD:\nname 'abc' is not defined",))
      

        Computing LegacyTarget for DescendantAddresses(directory='examples/tests')
          Computing LegacyTarget for examples/tests/java/org/pantsbuild/example/hello/greet:greet
            Computing LegacyTarget for examples/src/java/org/pantsbuild/example/hello/greet:greet
              Computing TargetAdaptor for examples/src/java/org/pantsbuild/example/hello/greet:greet
                Computing Struct for examples/src/java/org/pantsbuild/example/hello/greet:greet
                  Computing UnhydratedStruct for examples/src/java/org/pantsbuild/example/hello/greet:greet
                    Computing AddressFamily for Dir(path=u'examples/src/java/org/pantsbuild/example/hello/greet')
                      Throw(exc=MappingError(u"Failed to parse examples/src/java/org/pantsbuild/example/hello/greet/BUILD:\nname 'abc' is not defined",))
          Computing LegacyTarget for examples/tests/scala/org/pantsbuild/example/hello/welcome:welcome
            Computing LegacyTarget for examples/src/scala/org/pantsbuild/example/hello/welcome:welcome
              Throw(exc=MappingError(u"Failed to parse examples/src/java/org/pantsbuild/example/hello/greet/BUILD:\nname 'abc' is not defined",))
      
    3. I don't think you'd need to check for fully identical output - just that it's correctly identifying the designated leaf nodes for a somewhat complex trace. this could probably be done with a few well aimed self.assertRegexpMatches() calls.

      also, rather than an integration test where the output may be less than deterministic - you could also mock out the value of self._nodes and unit test throw() against a stable test node list.

  2. src/python/pants/engine/scheduler.py (Diff revision 1)
     
     
    
      
  3. src/python/pants/engine/scheduler.py (Diff revision 1)
     
     
     

    you should avoid using escaped newlines under almost all circumstances.

    here I'd recommend stacking the inputs to .format like so, which also makes the parameters easier to read/visually distinguish from one another:

    return '...'.format('  ' * level,
                        node.product.__name__,
                        ...)
    
  4. src/python/pants/engine/scheduler.py (Diff revision 1)
     
     
     

    this should probably get a newline after the inline function for readability.

  5. src/python/pants/engine/scheduler.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    seems to me like this whole bit could be done in a single iteration of _trace(self._nodes[root], 1) and without the list/enumeration/indexing, no?

    1. done in single iteration.

  6. 
      
WI
NH
  1. Looks pretty good. I'd like to see a test that specifically exercises the trace output though. It looks like there's only one that explicitly calls it, which is in test_graph.py

  2. src/python/pants/engine/scheduler.py (Diff revision 2)
     
     

    This comprehension might be a little confusing since it shadows entry. Could you either move the fn def up, next to is_bottom, or rename entry inside it?

    1. Thanks. rename to child_entry and parent_entry

  3. 
      
WI
WI
ST
  1. 
      
  2. tests/python/pants_test/engine/test_graph.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    I don't think this test is testing what you think.

    In this particular case, the leaf (used to be?) a Noop, which caused a Throw in the task that was depending on it. So there probably aren't any Throws in this trace.

    You might consider adding another test that does actually contain a Throw.

    1. (...or maybe I'm wrong. But testing another case that doesn't involve the cycle/Noop would be good.)

    2. Added more checks on
      test_type_mismatch_error
      test_not_found_but_family_exists
      test_not_found_and_family_does_not_exist

      It did have Throw covered in self.assertEqual(type(state), Throw)

        Computing Struct for graph_test:direct_cycle
          Computing Struct for graph_test:direct_cycle
            Computing Struct for graph_test:direct_cycle_dep
              Throw(exc=ValueError(u"No source of...
            Computing Struct for graph_test:direct_cycle_dep
              Noop(msg=u"Edge would cause a cycle:...
      
  3. 
      
WI
KW
  1. 
      
  2. src/python/pants/engine/scheduler.py (Diff revision 5)
     
     
     

    should this function un-nest and move up to become a peer of is_bottom and _format for readability?

    1. thanks! moved up and clearer now.

  3. src/python/pants/engine/scheduler.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     

    how about building up a string and then returning that to kill the duplication here?

    e.g.

    output = '{}Computing {} for {}'.format('  ' * level,
                                            entry.node.product.__name__,
                                            entry.node.subject)
    if is_one_level_above_bottom(entry):
      output = '\n{}{}'.format(...)
    
    return output
    
    1. s/output = /output += /g

    2. corrected.

  4. 
      
NH
  1. 
      
  2. It'd be better to use assertIn here.

  3. tests/python/pants_test/engine/test_graph.py (Diff revision 5)
     
     
     
     
     

    If these fail, the failure message won't be very useful. Could you adjust this so that the message is something like the comment above?

    1. corrected. extracted assert_equal_or_more_indentation to generate the error

      error looks like below if failed:

      E   AssertionError: 
      E   "    Throw(exc=ResolveError(u'A Struct was not found in namespace this/dir/does/not/exist for name "exist". Did you mean one of?:\n  ',))"
      E   should have more equal or more indentation than
      E   "      Computing UnhydratedStruct for this/dir/does/not/exist:exist"
      
  4. 
      
WI
WI
Review request changed

Status: Closed (submitted)

Change Summary:

cdb72c853b0d301dd95c43e2def10cb2da95feeb thanks gents!

Loading...