MAINT Breaking: Migrating target and scorer registries#2087
Open
rlundeen2 wants to merge 5 commits into
Open
Conversation
…es (Phase 4) Move TargetRegistry and ScorerRegistry off the legacy instance-only RetrievableInstanceRegistry stack onto the unified Registry + .instances shape already used by ConverterRegistry. Both registries now build instances by name (LLM scorers resolve a chat_target registry name; composite scorers and RoundRobinTarget resolve a list of names) and hold pre-configured instances under .instances. Per the no-deprecation decision, the old singleton surface (register_instance / get_instance_by_name / reset_instance) is removed entirely and every call site is updated to the .instances.* pattern in the same change. - Add Param build markers/aliases to scorer/target identifiers (chat_target, scorers, targets) - Make reference resolution list-aware; collapse the resolver getter to a flat ComponentType -> Registry map; move the resolvability gate onto the Registry base - Add TargetRegistry/ScorerRegistry under pyrit/registry/components with TargetMetadata/ScorerMetadata - Update all production consumers, tests, and docs/notebooks Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… scenarios notebook Address review findings on the Phase 4 target/scorer registry migration: - microsoft#3: _resolve_registry_reference now raises a clear ValueError when a list-annotated reference receives a non-list value (and vice versa), instead of constructing the component with the wrong argument shape and failing obscurely downstream. Docstring Raises updated. - microsoft#4: add tests for pre-built instances passed inside a list (passthrough), mixed name/instance lists, and the new shape-mismatch guards, for both the target and scorer registries. - #1: refresh the stale generated output in doc/code/scenarios/0_scenarios.ipynb that still referenced the removed TargetRegistry.register_instance, matching the already-migrated source docstrings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The legacy RetrievableInstanceRegistry had zero production subclasses after targets and scorers moved onto the unified Registry + .instances stack. Its get/get_entry/get_all_instances helpers are fully provided by the new-stack DefaultInstanceRegistry. Remove the class and its exports; BaseInstanceRegistry stays for AttackTechniqueRegistry. Repoint the legacy-base tests' ConcreteTestRegistry onto BaseInstanceRegistry with inline retrieval helpers (a test double) so BaseInstanceRegistry coverage is preserved, and drop the obsolete Base/Retrievable split assertions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Eliminate the ~95% duplication across TargetRegistry, ScorerRegistry, and ConverterRegistry by moving the shared mechanics into the base Registry: - Default _discover() scans _discovery_package().__all__ for concrete _base_type() subclasses (skipping the base and abstract classes via inspect.isabstract) and registers each by class name. - _get_registry_name default snaps to the class name (was snake_case); registries with a different scheme override it. - New _base_type()/_discovery_package() hooks (NotImplementedError defaults) let registries with bespoke discovery override _discover() instead. The three component registries now only supply _base_type, _discovery_package, _identifier_type, _metadata_class, plus their .instances container and any projected metadata properties. Abstract-base skipping is now uniform across all three. Updated the base Registry test to the class-name default. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…r service test The converter-service mock helper set `mock_target.__class__.__name__ = 'MockChatTarget'` on a `MagicMock(spec=PromptTarget)`. A spec'd mock returns the real spec class from `__class__` (so isinstance works), so this mutated the global `PromptTarget.__name__` permanently. Any later test reading that name — e.g. TargetRegistry's instance-type rejection message — then saw 'MockChatTarget', making test_register_instance_rejects_non_target fail in the full suite. The mock's identifier already carries class_name='MockChatTarget' via get_identifier(), so the line was redundant; remove it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is Phase 4 of the unified registry refactor (see the design and phase breakdown in this gist). It migrates the
TargetRegistryandScorerRegistryoff their bespoke legacy base and onto the unifiedRegistry+.instancesshape that the converter registry already uses, so every component registry now shares one consistent registration, retrieval, and resolution surface.It is an intentional breaking change with no deprecation shims — rather than bridging the old API. The state if we include the old API is very confusing, with a lot of classes, and it's difficult to tell which calls what.