MAINT: Simplifying scenario class vars#1784
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
[disclaimer - a GHCP client wrote this:]
Review: MAINT: Simplifying scenario class vars
Good refactoring — moving from abstract classmethods to constructor parameters is cleaner and eliminates the need for the registry to know about the classmethod protocol. The @cache-decorated module-level builders are a nice improvement over the _cached_strategy_class ClassVar pattern.
A few observations:
1. scenario_run_service.py — �ssert for runtime narrowing
\\python
assert instance_for_introspection is not None
strategy_class = instance_for_introspection._strategy_class
\\
These \�ssert\ statements are purely for type-narrowing (the preceding \if\ guarantees non-None), so they're fine — but worth noting they'd be stripped under \python -O. If this code is ever used in an optimized runtime, consider an explicit \if ... is None: raise\ instead. Low priority since PyRIT unlikely runs with -O.
2. \AdversarialBenchmark\ — deferred validation
Making \�dversarial_models\ optional and deferring validation to _get_atomic_attacks_async\ is a reasonable tradeoff for introspection support. The error message at line ~1400 is clear about what's required. 👍
3. Minor: _build_benchmark_strategy\ indirection
The static method \AdversarialBenchmark.build_benchmark_strategy()\ now just delegates to the module-level _build_benchmark_strategy(). This extra hop is fine for backward compat (the \�enchmark/init.py\ _getattr\ references it), but could eventually be simplified to just reference the module-level function directly from _init_.py.
4. Removed test for error handling in \LoadDefaultDatasets\
The old \ est_initialize_async_handles_scenario_errors\ test is removed. Error handling is now pushed into \ScenarioRegistry._build_metadata\ (which emits degraded metadata on failure). The coverage there looks adequate via the \logger.warning\ path in the registry, but you may want a dedicated unit test for the degraded-metadata fallback in _build_metadata\ if one doesn't already exist.
Overall: Clean simplification, tests appropriately updated. LGTM with the minor notes above.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ggles # Conflicts: # pyrit/scenario/scenarios/airt/__init__.py # pyrit/scenario/scenarios/airt/content_harms.py # pyrit/scenario/scenarios/benchmark/adversarial.py
- scenario_run_service.py: collapse two-pass introspection into single-pass to drop assert statements used for type-narrowing. - adversarial.py / benchmark/__init__.py: drop AdversarialBenchmark._build_benchmark_strategy staticmethod indirection; call module-level _build_benchmark_strategy directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds targeted unit tests for behavior introduced by the scenario-classmethod simplification: - ScenarioRunService: scenario class not no-arg instantiable raises ValueError; valid strategies forwarded to initialize_async; max_dataset_size with no dataset_names overrides default config in place. - AdversarialBenchmark: deferred adversarial_models validation when constructed for introspection. - PreloadScenarioMetadata: initialize_async warms cache via list_metadata and propagates registry errors. - airt / benchmark packages: lazy __getattr__ resolution for dynamic strategy enums; unknown attributes raise AttributeError. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2 participants
Making it simpler to initialize Scenarios. Class methods are no longer needed and this is nice because we were using the registry