Skip to content

REFACTOR: Promote backend service & CLI client defaults to ClassVar - #1971

Closed
Roman Lutz (romanlutz) wants to merge 1 commit into
microsoft:mainfrom
romanlutz:romanlutz/refactor-backend-cli-classvar-defaults
Closed

REFACTOR: Promote backend service & CLI client defaults to ClassVar#1971
Roman Lutz (romanlutz) wants to merge 1 commit into
microsoft:mainfrom
romanlutz:romanlutz/refactor-backend-cli-classvar-defaults

Conversation

@romanlutz

Copy link
Copy Markdown
Contributor

REFACTOR: Promote backend service & CLI client defaults to ClassVar

PR B-6 of the constants-audit cleanup, following the pattern established
in #1964 (backend / registry / setup) and #1965 (prompt_converter).

What changed

Promotes hard-coded numeric defaults across pyrit/backend/services/*.py
and pyrit/cli/api_client.py from method signatures (and one module-level
constant) onto their owning class as ClassVar constants, per the
style-guide rule
that constants live on the owning class, not at module scope.

Because Python signature defaults can't reference class attributes, the
defaults use the sentinel pattern:

class Foo:
    DEFAULT_LIST_LIMIT: ClassVar[int] = 50

    async def list_foo_async(self, *, limit: int | None = None) -> ...:
        if limit is None:
            limit = self.DEFAULT_LIST_LIMIT

Backend services (pyrit/backend/services/)

File Change
attack_service.py list_attacks_async(limit=20)AttackService.DEFAULT_LIST_LIMIT = 20
initializer_service.py list_initializers_async(limit=50)InitializerService.DEFAULT_LIST_LIMIT = 50
scenario_service.py list_scenarios_async(limit=50)ScenarioService.DEFAULT_LIST_LIMIT = 50
scenario_run_service.py list_runs(limit=100)ScenarioRunService.DEFAULT_LIST_LIMIT = 100; module-level _DEFAULT_MAX_CONCURRENT_RUNS = 3ScenarioRunService.DEFAULT_MAX_CONCURRENT_RUNS = 3 (constructor + get_scenario_run_service() factory updated to reference it)
target_service.py list_targets_async(limit=50)TargetService.DEFAULT_LIST_LIMIT = 50

CLI client (pyrit/cli/api_client.py)

Default New ClassVar
request_timeout=None60.0 literal PyRITApiClient.DEFAULT_REQUEST_TIMEOUT = 60.0
list_scenarios_async(limit=200) PyRITApiClient.DEFAULT_LIST_LIMIT = 200
list_initializers_async(limit=200) same DEFAULT_LIST_LIMIT (same value)
list_targets_async(limit=200) same DEFAULT_LIST_LIMIT (same value)
list_scenario_runs_async(limit=100) PyRITApiClient.DEFAULT_SCENARIO_RUNS_LIST_LIMIT = 100 (different value, named specifically)

Test update

tests/unit/backend/test_scenario_run_service.py swapped the now-removed
module-level _DEFAULT_MAX_CONCURRENT_RUNS import for
ScenarioRunService.DEFAULT_MAX_CONCURRENT_RUNS.

Out of scope

  • pyrit/backend/middleware/, target_service.py:_AZURE_ML_SCOPE,
    converter_service.py:_DATA_TYPE_EXTENSION — already covered by REFACTOR: Promote single-class module constants to ClassVar (backend + registry + setup) #1964.
  • Module-level class registries (_CONVERTER_CLASS_REGISTRY,
    _TARGET_CLASS_REGISTRY), _SIMPLE_TYPES, hostname-suffix tuples
    in target_service.py — explicitly skipped per the audit rules
    (cross-module / import-time registries stay at module scope).
  • The per-call polling timeout tuple
    httpx.Timeout(connect=10.0, read=None, write=30.0, pool=10.0) in
    get_scenario_run_async — internal method-specific tuning, not a
    configurable default.

No behaviour change

Pure structural rename. Same literal values, just moved onto the owning
class. The sentinel pattern preserves the public signature behaviour
exactly: passing nothing still selects the documented default; passing
an explicit value (including the old default) still works.

Verification

  • uv run --link-mode=copy ruff check pyrit/backend/ pyrit/cli/ tests/unit/backend/ tests/unit/cli/ ✅ All checks passed
  • uv run --link-mode=copy ty check pyrit/backend/ pyrit/cli/ — only a single pre-existing diagnostic in scenario_run_service.py:505 (updated_at=scenario_result.completion_time reported as datetime | None vs datetime); identical issue exists on main (line shifted by the ClassVar additions), unrelated to this PR
  • uv run --link-mode=copy pytest tests/unit/backend/ tests/unit/cli/ -x --no-header -q876 passed, 5 skipped

Moves hard-coded `limit` defaults across `pyrit/backend/services/*.py`
and `pyrit/cli/api_client.py` from method signatures (and one
module-level constant in `scenario_run_service.py`) onto their owning
class as `ClassVar` constants, using the sentinel-default pattern
(`limit: int | None = None` + `if limit is None: limit = self.DEFAULT_LIST_LIMIT`).
Also promotes the CLI client's `60.0` request timeout to
`PyRITApiClient.DEFAULT_REQUEST_TIMEOUT`.

No value changes. No behaviour changes.

Verified: ruff / ty / pytest tests/unit/backend tests/unit/cli (876 passed).

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