Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make precomputing fail more usefully #7994

Merged

Conversation

Projects
None yet
5 participants
@blorente
Copy link
Contributor

commented Jul 2, 2019

Problem

DaemonPantsRunner precomputes the v2 graph products at creation time (before run). If that raised an exception, it would store a return code of 1, run normally, and then exit with 1 at the end:

class DaemonPantsRunner:
...
  def run(self):
    ....
    else:
        self._exiter.exit(self.**exit_code** if self.exit_code else PANTS_SUCCEEDED_EXIT_CODE)

This hints that the behaviour that we want is to actually fail the run if the precomputation fails. If that is the case, we would like to fail more usefully, with a real exception.

Solution

Create a wrapper exception type that wraps whatever error happened in the precomputation. Then, in DPR.run(), re-raise this deferred exception immediately after setting up logging and stdio channels.

Result

What before seemed to succeed and returned 1, should now return a proper exception.

@blorente blorente requested review from stuhood, ity and cosmicexplorer Jul 2, 2019

@@ -102,6 +102,21 @@ def exit_code(self):
return self._exit_code


class _PantsProductPrecomputeFailed(Exception):

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 2, 2019

Member

So, at a fundamental level, there is no need to "precompute" or "warm" anything anymore. So we should actually be able to delete the warming codepath entirely.

This comment has been minimized.

Copy link
@blorente

blorente Jul 2, 2019

Author Contributor

Added a TODO :)

@stuhood

stuhood approved these changes Jul 2, 2019

Copy link
Member

left a comment

Thanks. Would be good to revisit this in a form that we're less worried about cherry-picking.


# TODO This can probably be moved to the try:except block in DaemonPantsRunner.run() function

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 2, 2019

Member

Linking to a ticket that explains why, or expanding the description here would be good. There is definitely dead code in here.

This comment has been minimized.

Copy link
@blorente

blorente Jul 3, 2019

Author Contributor

Opened #8002, I hope it's enough for most people to pick up, but would definitely love feedback to make it more approachable.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

Oh no, just noticed the needs cherry-pick label 😢 this can't use Py3 features like f-strings if this needs to be included in 1.17.x.

@ity

ity approved these changes Jul 2, 2019

Copy link
Contributor

left a comment

ah, looks like you have enough comments re: py2 vs 3 - lgtm otherwise!

Show resolved Hide resolved src/python/pants/bin/daemon_pants_runner.py Outdated

@blorente blorente force-pushed the blorente:blorente/make-precomputing-fail-properly branch from dce73e1 to 4470df9 Jul 3, 2019

@blorente

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

I added some unit tests for these specific cases, if CI is green I will merge tomorrow.

@blorente blorente merged commit f44f2c0 into pantsbuild:master Jul 5, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@blorente blorente deleted the blorente:blorente/make-precomputing-fail-properly branch Jul 5, 2019

blorente added a commit to blorente/pants that referenced this pull request Jul 5, 2019

Make precomputing fail more usefully (pantsbuild#7994)
**Problem**
DaemonPantsRunner precomputes the v2 graph products at creation time (before run). If that raised an exception, it would store a return code of 1, run normally, and then exit with 1 at the end:

class DaemonPantsRunner:
...
  def run(self):
    ....
    else:
        self._exiter.exit(self.**exit_code** if self.exit_code else PANTS_SUCCEEDED_EXIT_CODE)
This hints that the behaviour that we want is to actually fail the run if the precomputation fails. If that is the case, we would like to fail more usefully, with a real exception.

**Solution**
Create a wrapper exception type that wraps whatever error happened in the precomputation. Then, in DPR.run(), re-raise this deferred exception immediately after setting up logging and stdio channels.

**Result**
What before seemed to succeed and returned 1, should now return a proper exception.

blorente added a commit to blorente/pants that referenced this pull request Jul 8, 2019

Revert "Make precomputing fail more usefully (pantsbuild#7994)"
It introduced failures in the nightly cron (pantsbuild#8023), and a bunch more
errors when we tested it on Twitter internal targets.

blorente added a commit that referenced this pull request Jul 10, 2019

Revert "Make precomputing fail more usefully (#7994)" (#8024)
It introduced failures in the nightly cron (#8023), and a bunch more
errors when we tested it on Twitter internal targets.

illicitonion added a commit that referenced this pull request Jul 10, 2019

Revert "Make precomputing fail more usefully (#7994)" (#8024)
It introduced failures in the nightly cron (#8023), and a bunch more
errors when we tested it on Twitter internal targets.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.