FEAT: More Informative Attack Exceptions #1318
Conversation
rlundeen2
changed the title
| If an exception occurred, the context is preserved so that exception | ||
| handlers higher in the call stack can access it for enhanced error messages. | ||
| """ |
There was a problem hiding this comment.
nit could maybe add the Args: explanation for variables to keep consistency with the rest of the file
| # Build the "for X" part of the message based on execution context | ||
| for_clause = fn_name | ||
| try: | ||
| from pyrit.exceptions.exception_context import get_execution_context |
There was a problem hiding this comment.
nit should you import this at top of file?
| message=message, | ||
| conversation_id=context.session.adversarial_chat_conversation_id, | ||
| target=self._adversarial_chat, | ||
| with with_execution_context( |
There was a problem hiding this comment.
nit: this naming looks a little trippy the two "with" in a row but naming is hard so not a blocker by any means
|
|
||
| error_message = ( | ||
| f"Strategy execution failed for {exec_context.component_role.value} " | ||
| f"in {self.__class__.__name__}: {str(e)}{root_cause_info}\n\nDetails:\n{error_details}" |
There was a problem hiding this comment.
it's possible that error_details in an empty list in which case this output would look weird. maybe set a default value for error_details to be unknown
| objective_scorer=self._objective_scorer, | ||
| auxiliary_scorers=self._auxiliary_scorers, | ||
| role_filter="assistant", | ||
| with with_execution_context( |
There was a problem hiding this comment.
it looks like there's a few places this is added--send_prompt_async, score_response_async and score_async. Is there a reason we can't put the call within those functions ?
There was a problem hiding this comment.
I think the attack level is the lowest level where we still have this context. E.g. we can say "this is an error with the objective scorer" from here. But if we put it at send_prompt_async it doesn't know where it came from, we lose the attack_identifier, objective, etc.
| objective_scorer=self._objective_scorer, | ||
| auxiliary_scorers=self._auxiliary_scorers, | ||
| role_filter="assistant", | ||
| with with_execution_context( |
There was a problem hiding this comment.
nit: if we only ever call with_execution_context with a with statement, imo it's redundant to have the with_ at the beginning of the function name but also that might be convention
hannahwestra25
left a comment
hannahwestra25
left a comment
There was a problem hiding this comment.
nice change! my only concern is that it's easy to forget to add the with_execution_context when adding new attacks that don't inherit from prompt_sending or multi_prompt_sending but i'm also unsure how frequently we will be adding those
3 participants
This has something that has bugged me for a while, but I hadn't thought of a good solution for it. I think the solution here is decent, and it definitely improves usability. And I don't want perfect to get in the way of good...
Old way
The exceptions take some digging and often don't help.
e.g. here is a retry for a target. Which target? You'll never know...
Now, here is an error in objective_target, not that you would know that either....
New way
It still takes wrapping critical pieces in a handler, but I think it's already quite good. And will improve as we update identifiers to be better.
Here is a new error in objective_scorer:
Here is an error in a converter
Similar errors are available for both adversarial and objective chats. Here are new retry messages which now contain the component