-
Notifications
You must be signed in to change notification settings - Fork 664
FEAT: Attack Identifier #1364
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
base: main
Are you sure you want to change the base?
FEAT: Attack Identifier #1364
Conversation
| self._objective_target = objective_target | ||
| self._params_type = params_type | ||
| self._request_converters: list[Any] = [] | ||
| self._response_converters: list[Any] = [] |
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.
why not type List[PromptConverterConfiguration]?
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.
I could be talked into it, no super strong reason other than all usage right now is just converter lists. And we currently saved self._request_converters in the attacks but not PromptConverterConfiguration so it was a smaller change
| class_module=self.__class__.__module__, | ||
| objective_target_identifier=objective_target_identifier, | ||
| objective_scorer_identifier=scorer_identifier, | ||
| request_converter_identifiers=converter_identifiers or None, |
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.
For our data model should this be None or an empty list? I can imagine users wanting attack identifiers to explicitly reflect "no request converters were used" vs the more ambiguous "none were provided or found"
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.
None is consistent with how the other properties are populated, so I think it makes the most sense to keep for now. I know others aren't a list. So I have a slight pref for the way it is but don't care much.
| attack_identifier: Optional[Union["AttackIdentifier", Dict[str, Any]]] = None | ||
|
|
||
| # The identifier from the component's get_identifier() (target, scorer, etc.) | ||
| component_identifier: Optional[Dict[str, Any]] = None |
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.
This may be out of scope but why isn't this Union[TargetIdentifier, ScorerIdentifier, Dict[str, Any]]?
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.
I think as long as it's an Identifier it's okay here. What you say is more specific but I could see us adding more and using them. So slight pref as is but if you feel strongly I can change it!
Adding
AttackIdentifier, making executorsIdentifiableand removingLegacyIdentifiable