FEAT add TargetConfiguration & pieces#1573
FEAT add TargetConfiguration & pieces#1573hannahwestra25 wants to merge 9 commits intomicrosoft:mainfrom
Conversation
| """The resolved normalization pipeline.""" | ||
| return self._pipeline | ||
|
|
||
| def supports(self, *, capability: CapabilityName) -> bool: |
There was a problem hiding this comment.
do we need to differentiate between "natively supports" (i.e. this method) and "can support" , i.e. the current requires method ( which I also have a comment on maybe renaming)
I can imagine why someone might be interested to know if a target can handle their inputs natively, or needs some preprocessing (normalization) to handle it. Is that the idea behind having these 2 methods?
There was a problem hiding this comment.
Yes, the idea behind these two is to differentiate so I updated the naming to includes (per your other comment). I thought about renaming this one to natively supports but the "supports" seems redundant as you noted in your other comment. I renamed the other function as well and updated the description of this function. I could rename to natively_includes ? or something like that ? wdyt ?
There was a problem hiding this comment.
I like includes too for this, I think it does imply it is natively defined on the target and now easy to contrast with ensure_can_handle. Thank you!
…ra/add_target_config_pieces
| except KeyError as exc: | ||
| raise AttributeError(capability.value) from exc | ||
|
|
||
| def __getattr__(self, name: str) -> UnsupportedCapabilityBehavior: |
There was a problem hiding this comment.
should this return type be changed to NoReturn ?
| Returns: | ||
| list[Message]: The (possibly adapted) message list. | ||
| """ | ||
| result = list(messages) |
| try: | ||
| return self.behaviors[capability] | ||
| except KeyError as exc: | ||
| raise AttributeError(capability.value) from exc |
There was a problem hiding this comment.
NIT: why AttributeError? this is just a KeyError right?
| ) | ||
|
|
||
|
|
||
| def _default_normalizers() -> dict[CapabilityName, MessageListNormalizer[Message]]: |
There was a problem hiding this comment.
wonder if this can be called and saved as a constant instead of calling each time? Are there any race conditions (i.e. Normalizer state is modified across calls)?
| CapabilityName.MULTI_TURN: UnsupportedCapabilityBehavior.RAISE, | ||
| CapabilityName.SYSTEM_PROMPT: UnsupportedCapabilityBehavior.RAISE, |
There was a problem hiding this comment.
Does this need to be consistent with keys in _NORMALIZER_REGISTRY?
| CapabilityName.SYSTEM_PROMPT: _NormalizerRegistryEntry(order=0, normalizer_factory=GenericSystemSquashNormalizer), | ||
| CapabilityName.MULTI_TURN: _NormalizerRegistryEntry(order=1, normalizer_factory=HistorySquashNormalizer), |
There was a problem hiding this comment.
do we want to add a unit test or check that all the orders are unique?
| _ADAPT_ALL = CapabilityHandlingPolicy( | ||
| behaviors={ | ||
| CapabilityName.SYSTEM_PROMPT: UnsupportedCapabilityBehavior.ADAPT, | ||
| CapabilityName.MULTI_TURN: UnsupportedCapabilityBehavior.ADAPT, | ||
| CapabilityName.JSON_SCHEMA: UnsupportedCapabilityBehavior.RAISE, | ||
| CapabilityName.JSON_OUTPUT: UnsupportedCapabilityBehavior.RAISE, | ||
| CapabilityName.MULTI_MESSAGE_PIECES: UnsupportedCapabilityBehavior.RAISE, | ||
| CapabilityName.EDITABLE_HISTORY: UnsupportedCapabilityBehavior.RAISE, | ||
| } | ||
| ) | ||
|
|
||
|
|
||
| def _make_message(role: ChatMessageRole, content: str) -> Message: | ||
| return Message(message_pieces=[MessagePiece(role=role, original_value=content)]) |
There was a problem hiding this comment.
NIT: create shared conftest test fixtures?
jsong468
left a comment
jsong468
left a comment
There was a problem hiding this comment.
Thanks for a great design spec and clean implementation :)
3 participants
Description
This PR is 2 of ~5 PRs to introduce/integrate TargetConfiguration. This PR adds TargetConfiguration and the necessary pieces within it. This will replace the current pattern where capability checks, normalization, and error handling are scattered across targets, attacks, converters, and scorers.
NOTE: this just adds the classes, does not replace any of the current logic nor is it used anywhere in this PR
New classes:
Full spec: https://microsoft-my.sharepoint.com/:w:/p/hannahwestra/cQoo3V80Zd4FTafKPqjDmr4vEgUCBfTfToMrXUEegSaVhFPj2A
Tests and Documentation
added tests