Following up on the note @romanlutz left in #1867: the Scorer base class carries machinery that only LLM-backed scorers use, and it would be cleaner to separate that out. Opening this to propose a concrete shape and get direction before writing code.
The problem
Scorer currently owns several members that are only meaningful when the scorer calls an LLM:
_score_value_with_llm_async(...) (the JSON/parser plumbing, retries, system-prompt wiring)- the
chat_target parameter on __init__ and the TARGET_REQUIREMENTS validation it triggers
get_chat_target() and the _prompt_target attribute convention- the
prompt_target promotion inside _create_identifier
About half of the current scorers never use any of this. The regex family (RegexScorer, CredentialLeakScorer, PathTraversalOutputScorer, ShellCommandOutputScorer, SqlInjectionOutputScorer, XssOutputScorer) and the keyword family (AnthraxKeywordScorer, FentanylKeywordScorer, MethKeywordScorer, NerveAgentKeywordScorer), plus SubStringScorer and MarkdownInjectionScorer, are pure rule-based scorers. They inherit the LLM surface but never touch it.
The LLM-using scorers are the self_ask_* family and InsecureCodeScorer (11 in total). That split is stable and unlikely to change, so the concern is a good candidate for its own type.
Why a mixin rather than an intermediate base
The existing type hierarchy already discriminates on output shape: TrueFalseScorer vs FloatScaleScorer, and scorer_type keys off that via isinstance. "Uses an LLM" is orthogonal to "returns true/false vs float", so folding it into the linear hierarchy would force a combinatorial split (LLMTrueFalseScorer, LLMFloatScaleScorer, and so on).
A mixin composes cleanly instead:
class LLMScorerMixin:
"""Owns chat-target wiring and _score_value_with_llm_async for LLM-backed scorers."""
...
class SelfAskTrueFalseScorer(LLMScorerMixin, TrueFalseScorer):
...
class InsecureCodeScorer(LLMScorerMixin, FloatScaleScorer):
...The base Scorer keeps only what every scorer needs: the validator, the score_async / _score_piece_async interface, memory access, identifier construction, and the batch/evaluate helpers.
Proposed phasing
To keep each PR reviewable and avoid a single large diff:
- Introduce
LLMScorerMixin with the moved members, and have the LLM scorers inherit from it. Keep thin delegating shims on Scorer marked for 0.16.0 removal so nothing breaks for downstream code that subclassed Scorer directly and called _score_value_with_llm_async.
- Migrate the internal LLM scorers to the mixin and update their identifier construction.
- Remove the shims from
Scorer once the deprecation window closes.
Each phase is a separate PR. Phase 1 is additive and backward-compatible.
Open questions
- Mixin versus intermediate base: I lean mixin for the orthogonality reason above. If you would rather keep a linear hierarchy, that changes the shape substantially, so worth settling first.
- Does
TARGET_REQUIREMENTS and the chat_target validation move fully into the mixin, or does the base keep a no-op default so evaluate_async / batch helpers that read _prompt_target via getattr keep working unchanged?
- Naming:
LLMScorerMixin, LLMScorer, or something else that fits PyRIT conventions.
- Deprecation-shim approach in Phase 1 versus a single atomic move. The shim path is safer for downstream subclasses but adds temporary surface. Do you have a preference given where 0.16.0 is in its cycle?
Happy to start on Phase 1 once the mixin versus base question is settled.
Following up on the note @romanlutz left in #1867: the
Scorerbase class carries machinery that only LLM-backed scorers use, and it would be cleaner to separate that out. Opening this to propose a concrete shape and get direction before writing code.The problem
Scorercurrently owns several members that are only meaningful when the scorer calls an LLM:_score_value_with_llm_async(...)(the JSON/parser plumbing, retries, system-prompt wiring)chat_targetparameter on__init__and theTARGET_REQUIREMENTSvalidation it triggersget_chat_target()and the_prompt_targetattribute conventionprompt_targetpromotion inside_create_identifierAbout half of the current scorers never use any of this. The regex family (
RegexScorer,CredentialLeakScorer,PathTraversalOutputScorer,ShellCommandOutputScorer,SqlInjectionOutputScorer,XssOutputScorer) and the keyword family (AnthraxKeywordScorer,FentanylKeywordScorer,MethKeywordScorer,NerveAgentKeywordScorer), plusSubStringScorerandMarkdownInjectionScorer, are pure rule-based scorers. They inherit the LLM surface but never touch it.The LLM-using scorers are the
self_ask_*family andInsecureCodeScorer(11 in total). That split is stable and unlikely to change, so the concern is a good candidate for its own type.Why a mixin rather than an intermediate base
The existing type hierarchy already discriminates on output shape:
TrueFalseScorervsFloatScaleScorer, andscorer_typekeys off that viaisinstance. "Uses an LLM" is orthogonal to "returns true/false vs float", so folding it into the linear hierarchy would force a combinatorial split (LLMTrueFalseScorer,LLMFloatScaleScorer, and so on).A mixin composes cleanly instead:
The base
Scorerkeeps only what every scorer needs: the validator, thescore_async/_score_piece_asyncinterface, memory access, identifier construction, and the batch/evaluate helpers.Proposed phasing
To keep each PR reviewable and avoid a single large diff:
LLMScorerMixinwith the moved members, and have the LLM scorers inherit from it. Keep thin delegating shims onScorermarked for 0.16.0 removal so nothing breaks for downstream code that subclassedScorerdirectly and called_score_value_with_llm_async.Scoreronce the deprecation window closes.Each phase is a separate PR. Phase 1 is additive and backward-compatible.
Open questions
TARGET_REQUIREMENTSand thechat_targetvalidation move fully into the mixin, or does the base keep a no-op default soevaluate_async/ batch helpers that read_prompt_targetviagetattrkeep working unchanged?LLMScorerMixin,LLMScorer, or something else that fits PyRIT conventions.Happy to start on Phase 1 once the mixin versus base question is settled.