MAINT: Moving Identifiers to models#1858
Open
rlundeen2 wants to merge 10 commits into
Open
Conversation
… pyrit.models Copies the five identifier source files (component_identifier, atomic_attack_identifier, evaluation_identifier, class_name_utils, identifier_filters) from pyrit/identifiers/ to pyrit/models/identifiers/ with intra-package imports rewritten to the new path. The dataclass definition of ComponentIdentifier is byte-preserved. Re-exports all 18 public names from pyrit.models so consumers can write 'from pyrit.models import ComponentIdentifier'. The original pyrit/identifiers/ package is untouched in this commit; both packages coexist until later steps rewrite callers and install deprecation shims. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tifiers Mechanical literal-prefix replacement of 'pyrit.identifiers' to 'pyrit.models.identifiers' across all caller files. Covers both 'from pyrit.identifiers import X' and 'from pyrit.identifiers.submodule import Y' patterns, plus the converters instruction file and the doc/code/memory/5_advanced_memory notebook example. Internal pyrit.models data layer (attack_result, message_piece, score, scenario_result) now imports identifiers from within pyrit.models, removing the layering smell. The original pyrit/identifiers/ package is still present and untouched; it will be replaced with deprecation shims in step 5. Both packages coexist with byte-identical dataclass definitions, so no behavior changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
git mv of the five identifier test files from tests/unit/identifiers/ to tests/unit/models/identifiers/. Imports were already rewritten in step 3, so this is a pure file move plus a new __init__.py at the destination. tests/unit/identifiers/__init__.py is kept alive intentionally: step 5 will add test_deprecation_shim.py under that directory to cover the deprecation shim that replaces the original pyrit/identifiers/ package. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Each module now uses PEP 562 module-level __getattr__ to forward every public name to pyrit.models.identifiers.* and emit a one-shot DeprecationWarning per name per process. Added tests/unit/identifiers/test_deprecation_shim.py covering 57 cases: forwarding identity, one-warning-per-name, AttributeError on unknown names, stacklevel attribution, and both 'from X import Y' and attribute-access import styles. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ring Documents the move from pyrit.identifiers to pyrit.models.identifiers and references the deprecation timeline (removed in 0.16.0). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Auto-fix 187 I001 unsorted-imports introduced by caller rewrite - Auto-fix 1 D213 docstring style in pyrit/models/__init__.py - Add 'Any' return-type annotation to 6 shim __getattr__ functions (ANN202) - Convert bare attribute access to '_ = ...' assignment in 2 shim AttributeError tests (B018) Remaining ruff errors (4 D420) are pre-existing on the base commit and out of PR1 scope. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…evel PR1 mechanically moved the identifier sources under pyrit.models.identifiers but inadvertently broadened the pyrit.models top-level surface by 18 names (ComponentIdentifier, IdentifierFilter, the builder helpers, etc.). That broadening was not part of the Phase 2 PR1 scope and committing to it now would make pyrit.models the long-term home for these names even though the deliberate canonical import is pyrit.models.identifiers.* (matching how pyrit.models.seeds, pyrit.models.scenarios, etc. are addressed). Trim pyrit/models/__init__.py back to its pre-Phase-2 surface. Internal callers were already migrated to pyrit.models.identifiers.* in PR1, so this only affects hypothetical external users who would have started importing from pyrit.models directly during the brief window between PR1 landing and this fix - of which there are none. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PyRIT ships py.typed, so downstream type checkers (pyright, mypy) honor
type signatures from the pyrit.* package surface. Each pyrit.identifiers
shim file uses PEP 562 module-level __getattr__ to lazily forward to
pyrit.models.identifiers - but PEP 562 __getattr__ returns Any to type
checkers, so any external caller that still imports through the shim
(which is exactly the population the shim exists for) silently loses
static type information on every identifier symbol.
Add a guarded if TYPE_CHECKING: block at the top of each shim file
with explicit imports from pyrit.models.identifiers, mirroring the names
in __all__. This:
* Restores precise types in pyright/mypy without changing runtime
behavior (TYPE_CHECKING is False at runtime, so __getattr__ still
fires and emits the DeprecationWarning).
* Makes downstream IDEs (VS Code, PyCharm) show real signatures and
autocomplete instead of Any placeholders during the migration
window.
* Has zero impact on the shim's deprecation-warning behavior because
TYPE_CHECKING branches are stripped before module execution.
All 59 shim tests still pass, including the warn-once and stacklevel
regression tests.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…identifiers
The pyrit.identifiers shim exists only to keep external/downstream users
from breaking during the move to pyrit.models.identifiers. Internal
pyrit code and tests should always use the canonical pyrit.models.
identifiers.* path.
A runtime '-W error::DeprecationWarning:pyrit.identifiers' filter is
not a reliable guard because print_deprecation_message uses
stacklevel=3, which attributes the warning to the *caller's* module
(e.g. pyrit.executor.attack.foo) rather than to pyrit.identifiers.
It also misses dead-code regressions in modules that no test happens
to exercise (optional backends, auxiliary attacks, etc.).
Replace that brittle approach with a deterministic static-source scan:
test_no_internal_callers_of_deprecated_pyrit_identifiers_path
Walks pyrit/ and tests/ and asserts no .py file contains a
statement matching ^\\s*from pyrit\\.identifiers... or
^\\s*import pyrit\\.identifiers... outside the shim itself.
test_regression_guard_detects_a_deliberate_offender
Meta-test: hand the scanner each legitimate offender form (from,
import-with-submodule, import-as-alias, indented lazy import) and
confirm the regex matches, plus a set of look-alike strings and
comments to confirm there are no false positives. Without this
meta-test, a typo in the regex could let the guard pass
vacuously on a clean tree.
Both tests run in milliseconds and produce a clear file:line:source
message on failure - no need for contributors to remember a special
pytest invocation.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolved 4 conflicts: - pyrit/models/__init__.py: combined docstrings (main's data-layer/import-boundary rule first, then the pyrit.models.identifiers subpackage note). - pyrit/scenario/core/attack_technique_factory.py: kept main's 6 new imports and rewrote pyrit.identifiers -> pyrit.models.identifiers. - tests/unit/scenario/test_cyber.py: kept canonical identifier import; dropped PromptSendingAttack (only referenced in comments after main's refactor). - tests/unit/scenario/test_rapid_response.py: merged import sets, canonical identifier path, dropped OpenAIChatTarget/AttackTechniqueSpec (docstring-only). Rewrote 2 deprecated identifier imports in files newly added by main to the canonical pyrit.models.identifiers path: - pyrit/auxiliary_attacks/gcg/generator.py - tests/unit/executor/attack/multi_turn/test_pair.py Removed 4 stale entries from tests/unit/models/test_import_boundary.py that Phase 2 PR1 already cleared (the monotonic ratchet requires removing entries that no longer apply): attack_result + message_piece top-level entries, the score lazy entry, and the two identifier sub-entries under scenario_result (phase-7 scorer entries kept). Validation: deprecation-shim regression guard + import-boundary suite (64 passed), ruff at pre-existing 4-error baseline, full unit suite 8381 passed / 120 skipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
romanlutz
approved these changes
May 30, 2026
| from pyrit.models import PromptDataType | ||
| from pyrit.prompt_converter.prompt_converter import ConverterResult, PromptConverter | ||
| from pyrit.identifiers import ComponentIdentifier | ||
| from pyrit.models.identifiers import ComponentIdentifier |
Contributor
There was a problem hiding this comment.
both this line and the one above use longer imports.
from pyrit.models should be enough, and similarly from pyrit.prompt_converter
|
|
||
| # %% | ||
| from pyrit.identifiers.identifier_filters import IdentifierFilter, IdentifierType | ||
| from pyrit.models.identifiers.identifier_filters import IdentifierFilter, IdentifierType |
Contributor
There was a problem hiding this comment.
can we do pyrit.models only?
Contributor
There was a problem hiding this comment.
this is in every file, really.
Contributor
Author
There was a problem hiding this comment.
This is worth diving into!
I intentionally did the longer import here but open to changing.
With pyrit.models, it feels like a special module in that people may very well install it or use it solo. It'll he fairly big (in terms of number of classes and organization) but with no real dependencies.
So I was thinking of subclassing here. Eg models.identity. models.seeds. models.results.
Identity is the one that's coming up first though because we're moving it.
What do you think? I'm also okay taking this suggestion and exposing at the top like we do elsewhere.
Contributor
There was a problem hiding this comment.
The ones you mention are all other modules entirely. I'm just against exposing several levels deep because it means you have to maintain even the folder structure. That always comes back to bite you later on. I am happy with one or more modules if you have a preference but imports should be short
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
2 participants
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.
Moving identifiers to the model class as part of making that class simple data classes
https://gist.github.com/rlundeen2/3e8daa8e12a11b4b6e52587b3c9b1dca
This is most of phase 2. Still remaining is to convert the identifiers to pydantic