Repository navigation
FIX: Include output keys in SelfAskGeneral scorer identifiers (#3839) - #3847
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are reported for the changes that distinguish scorer identifiers by their output-key mappings.
No open issues 🎉
What changed in this PR
Fixes #3839 by including output-key mappings in both general self-ask scorer identifiers, preventing configurations that read different response fields from sharing an identifier.
Changes:
- Preserves all five output keys from an injected
JsonSchemaResponseHandleror constructor arguments. - Includes those keys in true/false and float-scale scorer identifiers.
- Adds tests for identifier differentiation, determinism, and injected-handler mappings.
| File | Description |
|---|---|
tests/unit/score/test_general_true_false_scorer.py |
Adds output-key identifier regression tests for the true/false scorer. |
tests/unit/score/test_general_float_scale_scorer.py |
Adds output-key identifier regression tests for the float-scale scorer. |
pyrit/score/true_false/self_ask_general_true_false_scorer.py |
Stores output keys and includes them in the true/false scorer identifier. |
pyrit/score/float_scale/self_ask_general_float_scale_scorer.py |
Stores output keys and includes them in the float-scale scorer identifier. |
Review effort: Balanced
f4c92b0 to
9262303
Compare
…oft#3839) Include score_value_output_key, rationale_output_key, description_output_key, metadata_output_key, and category_output_key in the identifier parameters of SelfAskGeneralTrueFalseScorer and SelfAskGeneralFloatScaleScorer, ensuring distinct identifiers when output keys vary. Signed-off-by: Hrushikesh Ramilla <ramillahrushikesh@gmail.com>
9262303 to
b0e476d
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
| self._metadata_output_key = response_handler._metadata_output_key | ||
| self._category_output_key = response_handler._category_output_key | ||
| else: | ||
| self._score_value_output_key = score_value_output_key |
There was a problem hiding this comment.
[P2] Include the output keys from callable handlers too
CallableResponseHandler also supports these five output keys, but it takes the else branch, which records the scorer's constructor arguments instead of the keys the handler actually reads. The same issue exists in the float-scale scorer.
For example, these handlers read opposite verdicts from the same parser output:
def parser(_):
return {"score_value": "true", "verdict": "false", "rationale": "r"}
first = CallableResponseHandler(parser=parser)
second = CallableResponseHandler(parser=parser, score_value_output_key="verdict")Passing them to two otherwise identical general true/false scorers still gives both scorers the same identifier, even though one reads true and the other reads false. This leaves the output-key collision unfixed for a supported handler.
Can we include the handler's effective output keys rather than limit this to JsonSchemaResponseHandler, and add a regression test for both scorers?
| if isinstance(response_handler, JsonSchemaResponseHandler): | ||
| self._score_value_output_key = response_handler._score_value_output_key | ||
| self._rationale_output_key = response_handler._rationale_output_key | ||
| self._description_output_key = response_handler._description_output_key | ||
| self._metadata_output_key = response_handler._metadata_output_key | ||
| self._category_output_key = response_handler._category_output_key |
There was a problem hiding this comment.
Could the response handler own its behavioral identifier, with that identifier included as a child of the scorer, rather than copying its five private fields into both scorers?
ComponentIdentifier already includes child hashes in the parent hash. The handlers also have _replay_identifier() contracts covering output keys and other parsing settings, which seems like useful prior art, although replay can opt out so that is not necessarily a drop-in solution. A handler-owned identity would cover callable and wrapped handlers consistently without each scorer needing concrete-type checks or knowledge of the handler's internals. Am I missing a reason to keep these settings on the scorer too?
Related API question, which could be a follow-up because it predates this PR: should an injected handler be mutually exclusive with explicitly supplied scorer-level output keys and response schema? For example:
response_handler=JsonSchemaResponseHandler(score_value_output_key="verdict"),
score_value_output_key="rating",The scorer-level "rating" setting is silently ignored. I'd expect a ValueError rather than accepting two sources of parser configuration. We would need an unset sentinel to distinguish omitted arguments from explicitly supplied defaults, and consider compatibility for existing callers.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Description
Fixes #3839.
SelfAskGeneralTrueFalseScorerandSelfAskGeneralFloatScaleScoreraccept five output key parameters (score_value_output_key,rationale_output_key,description_output_key,metadata_output_key, andcategory_output_key), either via constructor arguments or through an injectedJsonSchemaResponseHandler. However, these keys were not preserved or included in_build_identifier(), causing scorers with different output key mappings to produce identical identifiers.This change:
selfin both scorers, extracting them fromresponse_handlerwhen it is an instance ofJsonSchemaResponseHandleror falling back to the constructor arguments.score_value_output_key,rationale_output_key,description_output_key,metadata_output_key, andcategory_output_keytoparamswithin_build_identifier().Tests and Documentation
Added unit tests in
tests/unit/score/test_general_true_false_scorer.pyandtests/unit/score/test_general_float_scale_scorer.py:JsonSchemaResponseHandlerare reflected in the identifier.All 45 tests pass with 100% diff coverage.