[DEMONSTRATION ONLY - WILL NOT BE MERGED!] Component Config / Registry Entry: A new alternative design to Identifiers#1369
Open
bashirpartovi wants to merge 2 commits intoAzure:mainfrom
Open
Conversation
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
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.
Simplify Component Identity: ComponentConfig + RegistryEntry
NOTE:
For demonstration purposes, this is additive, I did not remove any of the existing Identifier functionality. Components implement both
IdentifiableandConfigurablefor side-by-side comparison. ForRegistryEntry, the metadata classes (ScenarioMetadata,InitializerMetadata) are swapped directly since they are ephemeral and never persisted to storage.Context
I was looking at the
Identifierclass and realized it's overly complex for what it does. It handles instance identity, registry metadata, hash computation with field exclusion annotations, serialization with storage truncation, and more, all in one class. It works, but the coupling it creates makes the codebase harder to change than it should be.I started asking what if we split this into two small, focused types instead? That's what this PR does. Below I'll walk through the specific issues I found and how the new design addresses each one.
Issue 1: Parents know the target's schema
This is the one that bothered me most. e.g. in
Scorer._create_identifier():The scorer is reaching into the target's
TargetIdentifierand cherry-picking fields it thinks are important (model_name,temperature,top_p). This is tight coupling, the scorer has to know the target's identity schema. If someone adds a field toTargetIdentifierthat matters for behavior (saymax_tokensorfrequency_penalty), the scorer's identity won't reflect it unless someone also remembers to update this extraction logic.And the extraction is lossy. The target has a full typed identifier with all its params; the scorer flattens it to a
Dict[str, Any]with only the fields someone decided to copy over.This isn't isolated to the scorer. The converter has the exact same coupling:
Same pattern, the parent reaches into the child's schema and cherry-picks the same four fields. Actually worse than the scorer version because it doesn't even check for
Nonevalues. Any component that holds a reference to another component and wants to include it in its identity has to repeat this extraction logic.With ComponentConfig, the scorer doesn't look inside the target at all:
The target's entire config is captured as a child. The scorer's hash incorporates the target's hash, if the target's config changes (new model, different temperature, anything), the scorer's hash changes automatically. The scorer never needs to know what fields the target has.
Issue 2: Hashing is implicit and annotation-driven
The current system uses field metadata annotations to control what gets hashed:
And in the subclasses:
Then hash computation iterates over all fields and checks each one:
This is clever, but it means that whether a field participates in hashing is determined by metadata on the field declaration, not by the structure of the data. When someone adds a new field to a subclass, they have to remember to annotate it correctly. There's no compiler error if you forget, the field just silently participates in (or is excluded from) the hash based on defaults.
The
_EXPANSION_CATALOGadds another layer,STORAGEimplicitly meansHASHtoo, so_ExcludeFrom.STORAGEactually excludes from both storage and hashing. You have to understand this expansion system to reason about what any given field does.With ComponentConfig, the separation is structural. There are only two buckets:
If it's in
params, it gets hashed. If it's not, it doesn't. There is no annotation system, no expansion catalog, no per-field metadata to get wrong. You can't accidentally put a field in the wrong bucket because the buckets are different structural locations, not metadata flags.Issue 3: Registry metadata inherits identity machinery it never uses
ScenarioMetadataandInitializerMetadataextendIdentifier:Through this inheritance,
ScenarioMetadatagets:_compute_hash(),hash,unique_name,pyrit_version,to_dict(),from_dict(),identifier_type,_ExcludeFromfield processing,_MAX_STORAGE_LENGTHtruncation. None of these are ever called onScenarioMetadatainstances - the registry builds them on-the-fly for CLI display and filtering, then discards them.This isn't just dead code on the instance. It's coupling. If someone changes how
Identifier._compute_hash()works, or howto_dict()serializes,ScenarioMetadatais implicitly affected. The hash is still computed in__post_init__even though nobody reads it. If someone adds a field toIdentifier,ScenarioMetadatainherits it. You can't tell from readingScenarioMetadatawhich parts ofIdentifierit actually relies on.The
_build_metadatacall sites also have to pass identity fields that are meaningless for class metadata:Every call passes
identifier_type="class". Every instance computes a hash that nobody reads. Every instance carriespyrit_versionandunique_namethat no consumer accesses. It's ceremony inherited from a base class designed for a different purpose.With RegistryEntry, the base class has exactly what class registries need and nothing more:
The metadata classes inherit only what they use:
And
_build_metadatadrops the dead fields:ScenarioMetadata(RegistryEntry)inherits 3 fields and 1 property. No hash computation, no serialization machinery, no version tracking, no exclusion annotations. Changes toComponentConfighashing or serialization can't affect registry metadata because they don't share a base class.Because these metadata classes are ephemeral, created by
_build_metadatafor CLI display and filtering, never persisted to a database, the base class swap is a direct replacement. No dual-implementation period needed, unlike theScorer/PromptTargetmigration where bothIdentifiableandConfigurablemust coexist while consumers migrate.The downstream consumers are unaffected:
format_scenario_metadata,format_initializer_metadata) accessessnake_class_name,class_name,class_description, and domain-specific fields, all present on the new types._matches_filtersis duck-typed viagetattrand works with bothIdentifierandRegistryEntry.BaseClassRegistry'sMetadataTbound changes fromIdentifiertoRegistryEntry.Issue 4: Subclass-per-component is a typed schema for what's really an arbitrary param bag
We have
ScorerIdentifier,TargetIdentifier, andConverterIdentifier, each extendingIdentifierwith their own fields. But look at what the subclasses actually add:These fields are the behavioral parameters of each component, which model, what temperature, which prompt template. They're configuration written once at construction, frozen, and then serialized to JSON or hashed. Nobody writes
identifier.scorer_type = "float_scale"during a running attack.The tell is the escape hatches. Both subclasses have a
*_specific_params: Optional[Dict[str, Any]]field, an untyped dict for "everything else that doesn't fit the schema."OpenAICompletionTargetuses it formax_tokens,frequency_penalty,presence_penalty, andn:Why do
temperatureandtop_pget typed fields butmax_tokensandfrequency_penaltygo into the untyped dict? There's no principled boundary, it's just which params someone happened to put on the base class first. The schema can't keep up with the variety of component params, so it falls back to a dict anyway.And the main consumers interact with identifiers through the base-class surface:
The one exception is
ConsoleScorerPrinter, which accesses subclass fields directly,scorer_identifier.target_info,.score_aggregator,.sub_identifier. But even there,target_infois alreadyDict[str, Any]accessed via.get("model_name"), andscorer_specific_paramsis iterated as a generic dict. It's half-typed at best. WithComponentConfigthis becomesconfig.params.get("score_aggregator")andconfig.children.get("sub_scorers", []), same access pattern, just throughparamsandchildreninstead of named fields.Nobody branches on
isinstance(identifier, ScorerIdentifier). The subclass-specific fields are written once at construction, serialized into a JSON blob in the DB, and queried by string key (scorer_class_identifier->>'scorer_type'). The type information is erased at the consumption boundary.When the fields are arbitrary component params, the subclasses already need untyped dict escape hatches, and the single consumer that does typed access is already half-untyped, the subclass hierarchy is providing a typed schema for data that doesn't benefit from one.
With ComponentConfig, this is explicit:
No escape hatch needed because the whole thing is a params dict. Components differ in what they put in
params, not in what subclass they instantiate. Adding a new component type means implementing one method, no newIdentifiersubclass with its ownfrom_dict()override.Issue 5: The Identifiable generic adds ceremony for no benefit
Components currently must declare their identifier type:
The generic type parameter
[ScorerIdentifier]looks like it gives type safety, but in practice callers almost always work with the result as a dict (to_dict()) or only access base-class fields (hash,class_name). The generic doesn't prevent misuse, it just adds boilerplate.And every
_build_identifierhas to manually passclass_name=self.__class__.__name__,class_module=self.__class__.__module__,class_description=...,identifier_type="instance", the same 4 lines repeated in every implementation.With Configurable, the boilerplate is gone:
ComponentConfig.of(self)extractsclass_nameandclass_modulefrom the object. No generic type parameter, no manual boilerplate fields.The new design
Two types, each with one job:
ComponentConfig - instance identity. A frozen dataclass with
(class_name, class_module, params, children, hash). ReplacesScorerIdentifier,TargetIdentifier,ConverterIdentifier, andIdentifiable[T].RegistryEntry - class metadata. A frozen dataclass with
(class_name, class_module, class_description)and asnake_class_nameproperty. ReplacesIdentifieras the base forScenarioMetadataandInitializerMetadata.These two types share no base class. They're connected only by duck typing -
_matches_filters()usesgetattrand works with both. Changes to one side can't break the other.Summary of how each issue is resolved
model_name,temperature,top_pfromTargetIdentifierComponentConfig, hash flows automatically_ExcludeFromenum,_EXPANSION_CATALOG, per-fieldmetadata={_EXCLUDE: ...}paramsgets hashed, everything else doesn'tScenarioMetadata(Identifier)inherits hash, serialization, exclusion logic,identifier_type="class"on every instanceScenarioMetadata(RegistryEntry)inherits 3 fields and 1 property; direct swap since metadata is ephemeralScorerIdentifier,TargetIdentifier,ConverterIdentifierwith typed schemas for arbitrary params (plus*_specific_paramsdict escape hatches)ComponentConfigtype, components differ in paramsIdentifiable[ScorerIdentifier], manualclass_name/class_module/identifier_typeConfigurableABC,ComponentConfig.of(self)extracts class infoOther things worth noting
Backward-compatible schema evolution. Adding an optional param with
Nonedefault doesn't change existing hashes becauseNonevalues are excluded:DB backward compatibility.
to_dict()inlines params at top level, so existing queries likescorer_class_identifier->>'scorer_type'keep working.from_dict()handles legacy__type__/__module__keys.Open/Closed for new components. Adding a new component type means implementing
_build_config()and putting params in a dict. No new identifier subclass, no newfrom_dict()override.Architecture
The design separates instance identity from class metadata, with no shared base class between them:
Why two unrelated types? Because they have fundamentally different lifecycles and consumers:
_build_metadatacalled for CLI/listingto_dict()/from_dict()for DB roundtrippyrit_versionfor schema evolution_matches_filtersusesgetattron both (duck-typed)Migration plan
Incremental, no big-bang:
ComponentConfig,Configurable,RegistryEntryadded. Zero breakage, both systems coexist.ComponentConfig.normalize()- classmethod that acceptsComponentConfig | dictand returnsComponentConfig, mirroring the existingIdentifier.normalize()pattern. Needed becauseMessagePieceuses.normalize()to handle both typed identifiers and legacy dicts from the DB.ConfigurableandIdentifiabletemporarily, verify hashes match.ScenarioMetadata(Identifier)→ScenarioMetadata(RegistryEntry),InitializerMetadata(Identifier)->InitializerMetadata(RegistryEntry),BaseClassRegistry'sMetadataTbound:Identifier->RegistryEntry. This is a direct replacement (no dual-implementation) because metadata objects are ephemeral, created by_build_metadatafor CLI display, never persisted. The only code change in_build_metadatacall sites is removingidentifier_type="class".MessagePiece- this is the largest consumer surface. Three typed identifier fields (converter_identifiers: List[ConverterIdentifier],prompt_target_identifier: Optional[TargetIdentifier],scorer_identifier: Optional[ScorerIdentifier]) change toComponentConfig.PromptMemoryEntry.get_message_piece()switches fromTargetIdentifier.from_dict()/ConverterIdentifier.from_dict()toComponentConfig.from_dict().ScenarioResult,Score,ConsoleScorerPrinter,ScorerRegistry,TargetRegistry, metrics IO.Identifierhierarchy,Identifiable, exclusion machinery.Deliberate simplification: The current
_MAX_STORAGE_LENGTHmetadata truncates long prompt templates into_dict()for storage display.ComponentConfigdoesn't replicate this - if a UI or printer wants to truncate long values, that's a display concern, not an identity concern. The hash already uses full values in both designs.