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

Support catching @rule errors #17911

Merged
merged 16 commits into from
Jan 4, 2023
Merged

Conversation

kaos
Copy link
Member

@kaos kaos commented Jan 2, 2023

Instead of returning any thrown exceptions for the execution request directly, support catching them in the rules during rule graph execution so recoverable/non critical errors may be handled.

This PR revives previous work by @cosmicexplorer in #10954 bringing it up-to-date with current main -- with some minor adjustments.

The primary difference is in the error handling; tracebacks rely more on the Exception.__cause__ attribute of the errors and rather than adding an additional layer of result wrapping for the nodes, we propagate all the Failures to the rules instead, so any exceptions thrown during rule execution is picked up as a Failure::Throw{} as before, but now it is forwarded to the parent rule to potentially catch. So, use raise .. from e in order to preserve the tracebacks of the previous error when desired.

Example:

@rule
async def catch_merge_digests_error(file_input: FileInput) -> MergedOutput:
    # Create two separate digests writing different contents to the same file path.
    input_1 = CreateDigest((FileContent(path=file_input.filename, content=b"yes"),))
    input_2 = CreateDigest((FileContent(path=file_input.filename, content=b"no"),))
    digests = await MultiGet(Get(Digest, CreateDigest, input_1), Get(Digest, CreateDigest, input_2))
    try:
        merged = await Get(Digest, MergeDigests(digests))
    except Exception as e:
        raise Exception(f"error merging digests for input {file_input}: {e}")
    return MergedOutput(merged)

@kaos kaos marked this pull request as ready for review January 3, 2023 15:04
@kaos kaos requested a review from stuhood January 3, 2023 15:04
Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really great! Thanks @kaos

src/rust/engine/src/externs/mod.rs Outdated Show resolved Hide resolved
src/python/pants/base/exceptions.py Show resolved Hide resolved
src/python/pants/engine/internals/selectors.py Outdated Show resolved Hide resolved
@kaos kaos merged commit dfbfd75 into pantsbuild:main Jan 4, 2023
@kaos kaos deleted the propagate-exception-stacktrace branch January 4, 2023 04:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants