Skip to content

REFACTOR: De-dup scorer output_key defaults onto Scorer base as ClassVars - #1970

Closed
Roman Lutz (romanlutz) wants to merge 1 commit into
microsoft:mainfrom
romanlutz:romanlutz/refactor-scorer-output-key-constants
Closed

REFACTOR: De-dup scorer output_key defaults onto Scorer base as ClassVars#1970
Roman Lutz (romanlutz) wants to merge 1 commit into
microsoft:mainfrom
romanlutz:romanlutz/refactor-scorer-output-key-constants

Conversation

@romanlutz

Copy link
Copy Markdown
Contributor

REFACTOR: De-dup scorer output_key defaults onto Scorer base as ClassVars

Scope

Part of the constants-audit cleanup (see PRs #1964 and #1965 for the established pattern). PR B-2 of the staged plan.

What changed

The same five string defaults for the LLM scoring JSON-response key names were duplicated across four sibling classes:

  • score_value_output_key = "score_value"
  • rationale_output_key = "rationale"
  • description_output_key = "description"
  • metadata_output_key = "metadata"
  • category_output_key = "category"

They are now declared once on the Scorer base as ClassVar[str]:

class Scorer(Identifiable, abc.ABC):
    DEFAULT_SCORE_VALUE_OUTPUT_KEY: ClassVar[str] = "score_value"
    DEFAULT_RATIONALE_OUTPUT_KEY: ClassVar[str] = "rationale"
    DEFAULT_DESCRIPTION_OUTPUT_KEY: ClassVar[str] = "description"
    DEFAULT_METADATA_OUTPUT_KEY: ClassVar[str] = "metadata"
    DEFAULT_CATEGORY_OUTPUT_KEY: ClassVar[str] = "category"

Touched files:

  • pyrit/score/scorer.py — promote constants; _score_value_with_llm_async now uses the sentinel pattern (str | None = None, resolved to self.DEFAULT_*_OUTPUT_KEY).
  • pyrit/score/float_scale/float_scale_scorer.py — override signature widened to str | None = None; the override forwards values through to super() which resolves the sentinel.
  • pyrit/score/float_scale/self_ask_general_float_scale_scorer.py — constructor now uses the sentinel pattern; self._*_output_key is resolved from the class default when no value is provided.
  • pyrit/score/true_false/self_ask_general_true_false_scorer.py — same constructor change.

Behavioural compatibility

  • SelfAskGeneralTrueFalseScorer() (no kwargs) still produces self._score_value_output_key == "score_value", etc.
  • SelfAskGeneralTrueFalseScorer(score_value_output_key="custom") still produces self._score_value_output_key == "custom".
  • Other internal callers of _score_value_with_llm_async that omit these args (e.g. SelfAskTrueFalseScorer, SelfAskRefusalScorer, SelfAskQuestionAnswerScorer, SelfAskCategoryScorer, SelfAskScaleScorer, SelfAskLikertScorer, InsecureCodeScorer) get the same resolved defaults as before.

No value changes. No behaviour changes.

Verified

  • uv run --link-mode=copy ruff check pyrit/score/ — clean.
  • uv run --link-mode=copy ty check pyrit/score/ — no new diagnostics (one pre-existing cv2 optional-import warning in video_scorer.py).
  • uv run --link-mode=copy pytest tests/unit/score/ -x --no-header -q1184 passed, 16 skipped.

…Vars

Promote the five repeated `*_output_key` string defaults
(`score_value`, `rationale`, `description`, `metadata`,
`category`) onto the `Scorer` base as `DEFAULT_*_OUTPUT_KEY:
ClassVar[str]`. `Scorer._score_value_with_llm_async`, `FloatScaleScorer`,
`SelfAskGeneralFloatScaleScorer`, and `SelfAskGeneralTrueFalseScorer` now
use the sentinel pattern (`str | None = None` + resolve to the class
default) instead of duplicating the literals.

No value changes. No behaviour changes.

Verified: ruff / ty / pytest tests/unit/score/ (1184 passed, 16 skipped).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@romanlutz

Copy link
Copy Markdown
Contributor Author

Closing per maintainer feedback on the constants-audit work.

This PR introduced the sentinel-default pattern (changing x: T = <literal> into x: T | None = None + if x is None: x = self.DEFAULT_X) which widens the public API signature. The preference is to keep constants inside their owning class (Track A — see #1964, #1965, #1972, #1973, #1974, #1975) but not to promote hard-coded single-use defaults via a None-sentinel.

Branch left intact in case any portion is worth cherry-picking later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants