FEAT: Adding Scorables as an entry point - #2400
FEAT: Adding Scorables as an entry point#2400Richard Lundeen (rlundeen2) wants to merge 13 commits into
Conversation
Give Scorer two inputs: a scorable (what to look at) and an expectation (what to look for). This lets a scorer answer questions that are not about a model response, which framework.md requires but the Message-only signature made impossible. This is a signature change, not a rewrite. A new MessageScorer intermediate base owns every message-shaped concern -- resolving a scorable to a Message, refusal and blocked-content substitution, piece validation, the role and error filters, the neutral fallback, and the exception wrapping around _score_async. The three direct Scorer subclasses re-parent onto it and no scorer body changes. - pyrit/models/score/ becomes a package holding every score type: the new scorables, ScoringExpectation and its conditions, ScoringScope, and the existing Score. - role_filter and skip_on_error_result move onto the message scorables, since both answer "which pieces count". - score_text_async and score_image_async keep their signatures and build a ContentScorable. Loose content stays ephemeral. - infer_objective_from_request is retired internally. ScorerEvaluator now reads each objective from the previous turn itself. - The legacy message-shaped parameters survive one release behind a DeprecationWarning, which the in-repo suite treats as an error. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d6f9fd6d-38b7-4841-bb94-c608033c1c04
Phase 1 shipped every scorable, condition, and scope type the design proposal names. Most of them have no consumer in this change and no phase in Part A (gist phases 1-6) that can construct or read one. The proposal itself says "nothing in Part A populates" a ScoringScope. Removed, with the phase that will reintroduce each: - SurfaceScorable and ScoringScope - phase 10, Part B, which is the first phase that populates a scope. - TraceScorable - phase 5, where it arrives with TraceSource and the Observation model it feeds. - ConversationScorable - never constructed in Part A. Phase 6 widens inside the scorer, as ConversationScorer reads conversation_id off the message today. - ToolCalled, ToolSequence, OutputMatches, Condition, and ScoringExpectation.conditions - phase 4 transports an expectation and phase 6 is the first reader. Until then a caller who sets conditions gets silence rather than an error, so ScoringExpectation holds objective alone. - ScoringExpectation.extra - named by no phase. - Volatility and the volatility ClassVars - nothing branches on them; their consumers are memoization in phase 3 and the lifecycle in phase 11. ScoringScope also carried a defect no consumer caught: window is declared tuple[datetime, datetime] | None and a test passed the string "last_turn". A frozen dataclass does not validate, so the test passed. Kept: MessageScorable, ContentScorable, MessageReferenceScorable, and ScoringExpectation. MessageReferenceScorable has a resolver and no producer, but phase 2 persists Score.scorable and a MessageScorable holding a live Message cannot round-trip through a column, so an id-based form is the persisted form. The unsupported-scorable test now uses a module-local dataclass instead of ConversationScorable, which proves the guard rejects anything outside _SUPPORTED_SCORABLES rather than one known sibling. No runtime behavior changes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d6f9fd6d-38b7-4841-bb94-c608033c1c04
MessageScorer did the resolving: an isinstance chain over a union of dumb records, plus a memory read and getattr duck-typing for the filter fields. That put data plumbing in the scorer and did not extend to the trace and surface scorables the design adds later. Scorables now resolve themselves. Scorable becomes an abstract root type instead of a union, SingleMessageScorable declares role_filter, skip_on_error_result, and an abstract resolve_message, and each concrete scorable says how it resolves: MessageScorable returns what it holds, MessageReferenceScorable reads memory, ContentScorable builds the never-persisted message. Adding a scorable is now adding a class, with no union to edit. Resolution needs pyrit.memory, which models.instructions.md does not allow on a model, so the scorables move from pyrit/models/score/ to pyrit/score/. That also clears a real cost: MessageScorable holds a Message but could not import one at runtime, because pyrit.models.messages imports ComponentIdentifierField back out of pyrit.models.score. ScoringExpectation stays in pyrit.models — it has no behavior, and a later phase has SeedGroup produce one. Only identity resolution belongs on a scorable. A scorer that derives a different kind of evidence from the same reference, such as trace ids or a file write, still does that widening itself. Removes _SUPPORTED_SCORABLES, the three-arm _resolve_message, and both getattr(scorable, ...) calls. This also carries three fixes from the review of the previous commit: - Scorer no longer defines the message hooks. _score_async, _score_piece_async, and _get_supported_pieces move to MessageScorer, and Scorer._score_scorable_async becomes abstract. A scorer deriving directly from Scorer that implemented only _score_piece_async used to build fine and then fail at score time with a confusing TypeError; it now fails at instantiation with a clear abstract-method error. - A partially resolvable MessageReferenceScorable raises and names only the ids that are missing, rather than only raising when nothing resolved. - The scorable module no longer needs a TYPE_CHECKING workaround for Message, since the circular import it worked around does not reach pyrit.score. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d6f9fd6d-38b7-4841-bb94-c608033c1c04
ContentScorable was made a SingleMessageScorable, which claimed it names exactly one Message. It does not. It carries content, and the Message it produced was fabricated on the spot as an adapter so message scorers could read it. The design proposal is explicit that a scorable is a reference and that loose content is the one exception (section 4), and that a later phase persists the content as a row of its own and retires the fabricated, never-persisted message (section 9.1). Putting it under that base also handed it role_filter and skip_on_error_result, which are meaningless for content that has no role and no error state. That was not only untidy: ContentScorable(value=..., role_filter="assistant") was accepted and then silently scored nothing, because the adapted piece is always role="user". The previous getattr duck-typing defaulted the filter to None, so this state was newly reachable. ContentScorable now derives from Scorable directly and exposes to_ephemeral_message instead of resolve_message. The different verb is the point: the message-shaped scorables return a message that already exists, while loose content becomes one. MessageScorer bridges the two shapes in two arms, and the content arm is marked as the transitional adapter it is. Passing a filter to ContentScorable is now a TypeError at construction. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d6f9fd6d-38b7-4841-bb94-c608033c1c04
Collapse the two message scorables into one by-id MessageScorable, per the gist. Use the from_message classmethods instead of a free helper. Correct the legacy removal version to 2.0.0. Rename _resolve_score_inputs to _consolidate_legacy_inputs. Deprecate extract_objective_from_previous_turn. Remove the dead objective ternary in Crescendo. Fail when a scorable names a piece id that memory does not hold. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d6f9fd6d-38b7-4841-bb94-c608033c1c04
The choice between loose content and a message reference was repeated in the legacy shim and in each of the three true/false wrappers. Move it to Scorer._scorable_for_message, which is the single bridge for the two callers that still hold a Message rather than the scorable it came from. The gist puts wrapper forwarding and the composite anchor fix in phase 2, and phase 1 changes no scorer body, so the wrappers keep resolving a message for now. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d6f9fd6d-38b7-4841-bb94-c608033c1c04
ScoringExpectation.objective is already optional, and a scorer reads it as expectation.objective if expectation else None, so an expectation holding no objective and no expectation at all mean the same thing. The ternary said otherwise in eight places and was dropped in a ninth, which made the callers look like they differed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d6f9fd6d-38b7-4841-bb94-c608033c1c04
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e5bf78a-8e94-4839-b6bf-7f29a6a19a38
Victor Valbuena (ValbuenaVC)
left a comment
Victor Valbuena (ValbuenaVC)
left a comment
There was a problem hiding this comment.
Looks great! I really like this PR, just have some questions about the implementation
| if len(message.message_pieces) == 1 and message.get_piece().not_in_memory | ||
| else MessageScorable.from_message(message) | ||
| ) | ||
| expectation = ScoringExpectation(objective=objective) |
There was a problem hiding this comment.
This PR introduces a ScoringExpectation that contains a single field objective. What are the pros and cons of this for composite scoring? This implementation uses the same expectation for each distinct scorer in the composite scorer. Do you think we should try to have a composite ScoringExpectation containing multiple objectives, one per subscorer, or something else?
There was a problem hiding this comment.
Fwiw I noticed in the gist you expanded on the concept in section 5 so I'm curious why we don't add those other fields in this PR
|
|
||
|
|
||
| @dataclass(frozen=True, kw_only=True) | ||
| class ContentScorable(Scorable): |
There was a problem hiding this comment.
What are the pros and cons of making MessageScorable decorate or otherwise use ContentScorable as a component? I like the idea of both inheriting Scorable (here's how to evaluate this piece of information) but I'm noticing that the fields in MessageScorable are a superset of ContentScorable (since a MessagePiece contains value and data_type.)
| scorable=( | ||
| ContentScorable.from_message(message) | ||
| if len(message.message_pieces) == 1 and message.get_piece().not_in_memory | ||
| else MessageScorable.from_message(message) | ||
| ), |
There was a problem hiding this comment.
I'm seeing these exact lines repeated a few times in this PR and I'm unsure about them. Firstly, because it's repeating the same basic idea across several scorers (treat this as ContentScorable if it's one piece and not in memory, otherwise treat it like MessageScorable). Secondly because this seems like a responsibility for score_async to just take a Scorable and figure out how to score it unless its caller specifically overrides it. I think the basic idea is great which is to distinguish between context-based scoring and content-based scoring, but I'm not sure about this implementation
| class ScoringExpectation: | ||
| """ | ||
| What a scorer scores against. | ||
|
|
||
| An expectation is a single parameter that attacks forward without inspecting it, | ||
| so a question authored in a technique configuration or a seed can reach a scorer | ||
| through an attack that knows nothing about it. | ||
| """ | ||
|
|
||
| objective: str | None = None |
There was a problem hiding this comment.
This may be a nit, but if ScoringExpectation exists to strongly type scoring objectives so that attacks forward them without needing to inspect them, why not just call this a ScoringObjective? Do you see this class evolving past this use (e.g. holding multiple objectives, confidence thresholds, etc.?)
There was a problem hiding this comment.
As I said earlier the gist expands on this significantly so I'm ok with this as-is but how do you see subsequent PRs extending this with conditions and/or other fields
| else: | ||
| legacy_message = cast("Message", message) | ||
| resolved_scorable = ( | ||
| ContentScorable.from_message(legacy_message) |
There was a problem hiding this comment.
why isn't this a message scorable ? don't we lose some attributes if we are creating content scorables from messages ?
There was a problem hiding this comment.
this is only if the message isn't in memory (not_in_memory = True), but I don't quite remember the situations in which this happens, maybe worth a comment
| if message is not None and scorable is not None: | ||
| raise ValueError("Pass either 'message' or 'scorable', not both.") | ||
| if message is None and scorable is None: | ||
| raise ValueError("Either 'message' or 'scorable' must be provided.") | ||
| if objective is not None and expectation is not None: | ||
| raise ValueError("Pass either 'objective' or 'expectation', not both.") | ||
| if message_options is not None and (role_filter is not None or skip_on_error_result is not None): | ||
| raise ValueError("Pass either 'message_options' or legacy message policy arguments, not both.") |
There was a problem hiding this comment.
nit: this is all validation so imo doesn't fit in a consolidate function
| async def _score_message_scorable_async( | ||
| self, | ||
| *, | ||
| scorable: Scorable, |
There was a problem hiding this comment.
kinda confused whether this function is just for scoring messages because it takes in a generic scorable but the comments all refer to messages. what about _score_message_compatible_scorable_async
|
|
||
| enforce_keyword_only_init(cls, base_name="Scorer") | ||
|
|
||
| def __init__(self, *, validator: ScorerPromptValidator, chat_target: PromptTarget | None = None) -> None: |
There was a problem hiding this comment.
we want a deprecation warning for this right ?
|
are you planning on updating the docs after all stages are completed ? |
| [ | ||
| piece.original_value | ||
| for piece in conversation | ||
| if piece.sequence == last_prompt.sequence - 1 and piece.original_value_data_type == "text" |
There was a problem hiding this comment.
Shouldn't this be == piece.sequence - 1 (and also check that piece.sequence>=1) in order to do what the docstring says, which is Read the text of the turn before an assistant message and use it as the objective.?
| skip_on_error_result: bool = False | ||
|
|
||
|
|
||
| def extract_objective_from_previous_turn(*, message: Message, memory: MemoryInterface) -> str: |
There was a problem hiding this comment.
when this gets removed, how will scorer_evaluator logic change (seems to be only consumer of this function)?
| from pyrit.prompt_target import CapabilityName | ||
| from pyrit.prompt_target.common.target_requirements import TargetRequirements | ||
| from pyrit.score import MessageScorable | ||
| from pyrit.score.message_scorer import MessageScoringOptions |
There was a problem hiding this comment.
Should we let MessageScoringOptions be exported from pyrit.score?
There was a problem hiding this comment.
curious why we do object validation for MessageScorable here as opposed to in the dataclass post_init? (e.g., things like >=1 piece ID, no duplicate IDs, valid UUIDs) and also similarly that the PromptDataType is valid for ContentScorable
Roman Lutz (romanlutz)
left a comment
Roman Lutz (romanlutz)
left a comment
There was a problem hiding this comment.
Great start for the scorer upgrades!
| scorable = ( | ||
| ContentScorable.from_message(message) | ||
| if len(message.message_pieces) == 1 and message.get_piece().not_in_memory | ||
| else MessageScorable.from_message(message) |
There was a problem hiding this comment.
Could we avoid converting this prepared Message back into persisted IDs? Some callers intentionally change the message content while retaining its original ID. For example, ConversationScorer combines the whole conversation into one string, and blocked-content handling substitutes partial content. The child scorer then reloads the original stored message and silently scores that instead. I think nested scorers need a way to receive the already-resolved message directly.
| ContentScorable: A scorable holding the converted message content. | ||
| """ | ||
| piece = message.get_piece() | ||
| return cls(value=piece.converted_value, data_type=piece.converted_value_data_type) |
There was a problem hiding this comment.
(GHCP comment) This conversion drops message-specific state such as the role and response_error; the resolver reconstructs the piece as a user message with no error. For example, an ephemeral blocked assistant response passed through the compatibility shim will no longer take SelfAskRefusalScorer's deterministic blocked-response path and can be scored as not refused. Could ephemeral messages use a lossless representation, leaving ContentScorable for genuinely loose content?
Wrapper scorers forwarded a prepared message by re-describing it as a scorable, so children reloaded the pre-substitution pieces from memory, or lost role and error state for a message that was never persisted. Add MessageScorer.score_message_async as the in-hand entry point and route the composite, inverter, threshold, and deprecated-message paths through it. Also: adapt pre-2.0 direct Scorer subclasses so they keep working; declare and enforce routable conditions instead of dropping them silently; read the request for the scored turn rather than the conversation's latest; export MessageScoringOptions and MessageScorableResolver; validate MessageScorable id tuples; migrate the remaining documentation call sites.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7affb573-e048-445a-9152-0b46e997dd89
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7affb573-e048-445a-9152-0b46e997dd89
…condition-routing
5 participants
Description
Phase 1 of the scorer contract proposal: https://gist.github.com/rlundeen2/cb2e3a14ade6159258e276bcecb707d9
A scorer now takes two inputs: a scorable (what to look at) and a ScoringExpectation (what to look for). This is a signature change, not a rewrite. No scorer body changes.
Introduce
Scorableand its variants; makescore_async(*, scorable, expectation)the entry point. AddScoringExpectation. Keep theMessagesignature as a deprecated wrapper with the mapping in §13, and add theMessageScorerintermediate base so no existing scorer body changes. Re-expressscore_text_async/score_image_asyncoverContentScorable. Migrate the in-repo call sites and make the deprecation warning an error in CI.Scorable = MessageScorable | ContentScorable.MessageScorablenames message piece ids and resolves itself through memory;ContentScorableholds a value that was never persisted. The scorable defines the scope that is scored.Wrapper scorers (composite, inverter, threshold) still resolve a
Messagerather than forwarding the scorable, because forwarding would bypass the exception wrapping, fallback scores, and ephemeral-link dropping that live inMessageScorer. The gist places that work, and the composite anchor fix, in phase 2.Tests and Documentation
tests/unit/score/test_scorable.pyandtest_message_scorer.pyare new.test_message_scorer.pycovers the deprecated path deliberately: one test per legacy input (positionalmessage,message=,objective=,role_filter=,skip_on_error_result=,infer_objective_from_request=), plus the §13 rule that the shim scores the supplied message rather than widening to its conversation, plus the conflicting-input guards. Each opts in withpytest.warns.Everything else is migrated.
pyproject.tomlturns this deprecation into an error for the test suite, so an unmigrated call fails CI. Full unit suite: 15373 passed, 6 skipped.Docs are not updated yet. Five files under
doc/still call the legacy path and will emit deprecation warnings until a follow-up pass. JupyText has not been run for this reason. Holding that pass until the code is reviewed.