[engine] Support tracebacks in engine traces; only show engine traces… #4549

Merged
merged 3 commits into from May 4, 2017

Conversation

Projects
None yet
3 participants
@baroquebobcat
Contributor

baroquebobcat commented May 4, 2017

… w/ flag

Problem

Tracebacks from within rules don't show up in engine traces when --print-exception-stacktraces is enabled. #4446

Solution

Capture tracebacks and carry them through the engine.

Also, only print traces when --print-exception-stacktraces is enabled. I could break this bit out.

Result

Traces include the exception trace. Traces are only shown when --print-exception-stacktraces is enabled.

Problem

(explain the context of the problem and why you're making this change. include
references to all relevant github issues.
)

Solution

(describe the modifications you've made.)

Result

(describe how your changes affect the end-user behavior of the system. this section is
optional, and should generally be summarized in the title of the pull request.
)

[engine] Support tracebacks in engine traces; only show engine traces…
… w/ flag

### Problem
Tracebacks from within rules don't show up in engine traces when --print-exception-stacktraces is enabled. #4446

### Solution
Capture tracebacks and carry them through the engine.

Also, only print traces when --print-exception-stacktraces is enabled. I could break this bit out.

### Result

Traces include the exception trace. Traces are only shown when --print-exception-stacktraces is enabled.
src/python/pants/engine/engine.py
@@ -111,8 +114,16 @@ def product_request(self, product, subjects):
# TODO: See https://github.com/pantsbuild/pants/issues/3912
throw_roots = tuple(root for root, state in result_items if type(state) is Throw)
if throw_roots:
- cumulative_trace = '\n'.join(self._scheduler.trace())
- raise ExecutionError('Received unexpected Throw state(s):\n{}'.format(cumulative_trace))
+ # TODO test these cases

This comment has been minimized.

@baroquebobcat

baroquebobcat May 4, 2017

Contributor

Need to figure out the appropriate tests here. Also, this implementation feels ugly. I could back out the trace eliding, but that doesn't feel quite right either.

@baroquebobcat

baroquebobcat May 4, 2017

Contributor

Need to figure out the appropriate tests here. Also, this implementation feels ugly. I could back out the trace eliding, but that doesn't feel quite right either.

@baroquebobcat baroquebobcat requested review from stuhood, kwlzn and JieGhost May 4, 2017

@stuhood

stuhood approved these changes May 4, 2017

+ if len(throw_root_states) == 1:
+ raise throw_root_states[0].exc
+ else:
+ raise ExecutionError('Multiple exceptions encountered:\n {}'

This comment has been minimized.

@stuhood

stuhood May 4, 2017

Member

Getting multiple failed roots here doesn't mean multiple failures, necessarily. Because a Throw in the graph with two "parent" nodes will fail both parents with the same Throw.

Deduping by the string value of the throw might work, but the real right way would be to find the lowest point in the graph, which is where the Throw originated (as trace does). Deduping is probably fine, but executing a different native method might be cleaner?

@stuhood

stuhood May 4, 2017

Member

Getting multiple failed roots here doesn't mean multiple failures, necessarily. Because a Throw in the graph with two "parent" nodes will fail both parents with the same Throw.

Deduping by the string value of the throw might work, but the real right way would be to find the lowest point in the graph, which is where the Throw originated (as trace does). Deduping is probably fine, but executing a different native method might be cleaner?

This comment has been minimized.

@baroquebobcat

baroquebobcat May 4, 2017

Contributor

Hm. That's true.
A few options:

  • dedup common errors
  • for each root, display what we were going after and the associated error message. This might still be too much information for the end user case though.

I'm going to defer this to another round error message improvements if that's ok.

@baroquebobcat

baroquebobcat May 4, 2017

Contributor

Hm. That's true.
A few options:

  • dedup common errors
  • for each root, display what we were going after and the associated error message. This might still be too much information for the end user case though.

I'm going to defer this to another round error message improvements if that's ok.

@@ -92,8 +92,8 @@ impl RawNode {
(RawStateTag::Empty as u8, externs::create_exception("No value")),
Some(Ok(v)) =>
(RawStateTag::Return as u8, v),
- Some(Err(Failure::Throw(msg))) =>
- (RawStateTag::Throw as u8, msg),
+ Some(Err(Failure::Throw(exc, _))) =>

This comment has been minimized.

@stuhood

stuhood May 4, 2017

Member

Carrying the rendered backtrace around like this feels redundant, although it's probably not a huge deal. Might be cleaner to have a python function to render an exception?

@stuhood

stuhood May 4, 2017

Member

Carrying the rendered backtrace around like this feels redundant, although it's probably not a huge deal. Might be cleaner to have a python function to render an exception?

This comment has been minimized.

@baroquebobcat

baroquebobcat May 4, 2017

Contributor

The problem is that python 2.7 doesn't attach the stack info to the exception. I could grab the stack and pass as a Value instead of a Buffer, and then render at render time, but I'm not sure it would be cheaper.

@baroquebobcat

baroquebobcat May 4, 2017

Contributor

The problem is that python 2.7 doesn't attach the stack info to the exception. I could grab the stack and pass as a Value instead of a Buffer, and then render at render time, but I'm not sure it would be cheaper.

@JieGhost

LGTM!

@baroquebobcat baroquebobcat merged commit 6513584 into pantsbuild:master May 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment