MAINT: Remove unused fields in MessagePiece#1616
MAINT: Remove unused fields in MessagePiece#1616behnam-o wants to merge 14 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Removes several unused MessagePiece fields and associated memory/query surface area, and updates tests/docs to match the simplified model.
Changes:
- Removed
originator,scorer_identifier, andtargeted_harm_categoriesfromMessagePieceserialization and memory storage. - Changed
scoreshydration to occur post-construction viaMessagePiece._set_scores()(memory layer + tests updated). - Removed
targeted_harm_categoriesfiltering capability fromMemoryInterface.get_attack_results()and DB-specific implementations.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
pyrit/models/message_piece.py |
Removes unused fields; introduces score hydration via _set_scores; updates to_dict() output. |
pyrit/memory/memory_models.py |
Drops targeted_harm_categories persistence and sets scores via _set_scores() when hydrating MessagePiece. |
pyrit/memory/memory_interface.py |
Removes targeted_harm_categories from get_attack_results() and deletes abstract hook for harm-category query conditions. |
pyrit/memory/sqlite_memory.py |
Removes SQLite harm-category attack-result filtering helper. |
pyrit/memory/azure_sql_memory.py |
Removes Azure SQL harm-category attack-result filtering helper. |
pyrit/score/conversation_scorer.py |
Stops copying removed originator field when creating derived pieces. |
tests/unit/models/test_message_piece.py |
Updates tests for removed fields and new score-hydration pathway; deletes scorer/harm-category tests. |
tests/unit/memory/memory_interface/test_interface_attack_results.py |
Updates helper signature and removes harm-category filtering tests. |
tests/unit/executor/attack/component/test_conversation_manager.py |
Updates tests to attach scores via _set_scores() instead of constructor. |
tests/integration/memory/test_azure_sql_memory_integration.py |
Removes integration test for harm-category filtering. |
tests/unit/prompt_normalizer/test_prompt_converter_configuration.py |
Changes how mock converter “name” is set. |
doc/code/memory/3_memory_data_types.md |
Removes docs for some fields (but not all—see comments). |
doc/code/memory/1_sqlite_memory.ipynb / doc/code/memory/9_exporting_data.ipynb / doc/code/executor/attack/1_prompt_sending_attack.ipynb |
Updates notebook outputs to reflect new schema/fields and new execution environment. |
| converter_identifiers: Optional[list[Union[ComponentIdentifier, dict[str, str]]]] = None, | ||
| prompt_target_identifier: Optional[Union[ComponentIdentifier, dict[str, Any]]] = None, | ||
| attack_identifier: Optional[Union[ComponentIdentifier, dict[str, str]]] = None, | ||
| scorer_identifier: Optional[Union[ComponentIdentifier, dict[str, str]]] = None, |
There was a problem hiding this comment.
why do we get rid of scorer_identifier but not the other identifiers ?
There was a problem hiding this comment.
Because scores should include it
There was a problem hiding this comment.
This particular one, along with other fields here, is just never populated in other classes - and this PR is only focusing on such unused fields that are never populated, so there's no point in having them.
Other ones could be in scope for removal, for example attack_identifier is also duplicated between message pieces and attack_results ... but the difference is, that is populated in other classes today, and will have a more cascading change all over the repo.
There are several fields in the message piece class that are never set or used. This PR removes them to make the constructor and objects cleaner.
There is a "scores" field, that gets populated during memory queries, and this behavior is kept as is, but replaced with a property with an internal setter.