-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
Add error boundary support for rules #17745
Conversation
Get(..., safe: bool)
support.
This makes me think of Rust's Let's say we had: _ValType = TypeVar("_ValType")
_ErrType = TypeVar("_ErrType")
@dataclass(frozen=True)
class Result(Generic[_ValType, _ErrType]):
@property
def maybe_value(self) -> _ValType | _ErrType:
...
@property
def value(self) -> _ValType:
val = self.maybe_value
# Some assertion that its the right type...
return val Then you could have your rules return async def my_rule(...) -> Result[TypeICareAbout]:
... so then it just becomes a question of how to make that "work" in the Pants' little type scraper, right? |
Also, can you give some links to in-Pants-repo examples of the behavior you're trying to improve upon? That'd help visual folks like me 😄 |
Thanks @thejcannon I'll have a proper look at your comments later today/tonight. Having stepped away for a minute, I realize I actually think this might work better (except perhaps if used in a @rule
async def i_want_this_to_be_unfallible(...) -> CertainResult:
try:
not_so_sure = await Get(Data, UnreliableSource(), _error_boundary=True)
...
except Exception as e:
return CertainResult(error=f"Oh no! ERROR: {e}")
...
return CertainResult(result="Yay!") That way there's no need to adjust the type hints at all, and things will just continue to work as now, except that any exceptions from invoked rules will be captured rather than propagate out as an engine error to the root result. |
The |
One thing this would allow, is to get rid of (or at least not require) the use of all the "description_of_origin" fields. The idea being, that in case of errors, you can throw the error with the information available at hand, and the callsite (the current origin for the value of This serves two things: one is to make requests slimmer and improve cacheability as we remove the request origin as the response will be the same regardless of where the request came from, and secondly it allows the origin/callsite to decide if it is a recoverable error which is not possible today with the I don't have a bigger example code snippet at hand right now, hope the above will shed some light on the motivation for this nonetheless. :) |
Btw, I do like the Edit: Ok, I see you could introduce thin "wrapper" rules around regular rules to capture any error.. but that seems more boilerplatey. |
Some prior art in this area: #10954 I believe that I would be in favor of having exceptions raised inside of But it's also worth noting that some error context will now be added automatically without catching/re-throwing by implementing |
Nice 👍🏽 I'll have to use that more. It does not solve the issue with allowing a rule to continue in face of errors though (due to bad input values or other issues)
👀 🙏🏽
Care to elaborate a bit on this point? |
i.e.: allowing for try-catch. Currently a |
Ah, right. Yes, so that's what I've implemented here then, as suggested in #17745 (comment) ( |
Looking at #10954 the idea here is pretty close to that, but with an opt-in for the new behaviour, and also doesn't mask the exception type but passes the original error on as-is. Maybe would be a good idea to wrap it in an engine specific error to be able to dress it with more context about the origins of the exception.. ? Could steal most of the tests however with only minor adjustments I think... :) |
let res = select.run_node(context).await; | ||
if get.safe { | ||
match res { | ||
Err(Failure::Throw { val, .. }) => Ok(val), | ||
_ => res, | ||
} | ||
} else { | ||
res | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generators have a generator.throw
method which should probably be used rather than sending an Exception
value via generator.send
: otherwise Get
s will not be able to request values which subclass Exception
and act as error boundaries (niche, I know).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, cool. Yea, I did consider that return values couldn't be based on Exception
, but was unaware of that solution. Neat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, careful with Exception v BaseException. I never know when it's appropriate to use Base instead of Exception.
Edit: ah I think it's exceptions that terminate don't derive from Exception ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, careful with Exception v BaseException. I never know when it's appropriate to use Base instead of Exception.
Edit: ah I think it's exceptions that terminate don't derive from Exception ?
Yea, I think I recall having read that Exception
is everything except SystemExit
and KeyboardInterrupt
pretty much..
@stuhood With this, I think I may be able to solve the need I have for #17674. If the |
And here is another example: https://github.com/pantsbuild/pants/pull/17347/files#r1059664930 |
Superseded by #17911 |
The idea: to allow
@rule
s to declare "error boundaries", by introducing a new_error_boundary: bool = False
param toGet()
s andEffect()
s. :) (using underscore prefix on_error_boundary
to indicate this as a private feature, at least for now ;) )Example:
Outdated example
This allows rules to not have to worry about wrapping return values as "optional foo" etc, which can be very invasive especially when the optional but is buried deep in the rule graph, all intermediate types must also support, and handle, the optional result values every where.
I'm still not sure how to adjust the type hints to accommodate the new result type of a "safe"Get
, ought to be_Output | Exception
to force consumers to address the possibility of the error in the result value.Edit: no need, throwing errors at the error boundary instead side-steps the typing issue altogether.
An alternative approach, presented by @thejcannon, could be to support a
Result
enum like of type where one leg is a the value and the other the error. Not sure what that would look like (in UX nor in implementation).