Skip to content

Presidio evaluator redesign towards v0.3 - #169

Closed
omri374 wants to merge 106 commits into
masterfrom
ralph/presidio-evaluator-redesign
Closed

Presidio evaluator redesign towards v0.3#169
omri374 wants to merge 106 commits into
masterfrom
ralph/presidio-evaluator-redesign

Conversation

@omri374

@omri374 omri374 commented Mar 26, 2026

Copy link
Copy Markdown
Collaborator

This PR replaces the tightly-coupled evaluate_all() monolith with a clean three-layer pipeline:
Model → CanonicalMapper → Evaluator.

Main changes:

  • The evaluator no longer owns model inference or entity mapping. Each step is an independent function call with a shared DataFrame as the interface. CanonicalMapper (new entity_mapping package) performs a four-tier auto-resolution (EXACT → COUNTRY → FUZZY → PENDING) that surfaces unknown labels explicitly instead of silently dropping them.
  • Non-Presidio model wrappers (FlairModel, SpacyModel, StanzaModel, AzureAITextAnalyticsWrapper) are removed; models should be added via Presidio's recognizer framework.
  • Tooling is modernized: Poetry → uv, ruff added, pre-commit hooks, Python minimum raised to 3.11.

Note to reviewers: There are 121 files changed. Please don't read them all. Start with the two ADRs (ADR-001, ADR-002) for design intent, then migration-guide.md cover all the new contracts. Run tests with uv run pytest -m "not integration" and review the notebooks (4,5,6) too.

What (almost) didn't change:

  • data_objects
  • data generator
  • notebooks 1,2,3
  • dataset formatters
  • experiment tracking

omri374 and others added 30 commits December 16, 2025 11:39
Updated dependencies for ner group and added version constraints for gliner and onnxruntime.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Removed deprecated check for entity_mapping in model.
- Add BIO/BIOES/BILOU/BILUO prefix/suffix stripping (_strip_bio)
- Auto-map O (outside token) to None tier
- Fix log messages to use ASCII -> instead of Unicode ->
- Fix UNRESOLVED log message (remove stale 'prompting user' suffix)
- Remove stale CANONICAL_DEPTH import from __init__.py
- Expand ALL_CANONICAL_ENTITIES to include intermediate hierarchy nodes (e.g. PERSON)
- Remove stale duplicate canonicalize() stub from hierarchy.py
- Update tests to use XYZZY_UNKNOWN (MY_UNKNOWN was auto-resolved via MY=Malaysia country prefix)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… implementation

US-002: map() method - already implemented, tests passing
US-003: resolve_interactively() - already implemented, tests passing
US-004: Add from_dataset() classmethod (extracts unique entity types from InputSample spans)
US-005: render_html() - already implemented, tests passing
US-006: Add missing test coverage:
  - TestBIOStripping (B-PERSON, PERSON-I, BANK_ACCOUNT unchanged, O token)
  - TestFromDataset (happy path dict, pending path instance, kwargs passthrough)
  - TestRenderHtml (no raise without IPython, callable before resolution)
- Fix ASCII log arrows in map() method (-> not Unicode ->)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Mapper API)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
omri374 added 14 commits March 25, 2026 10:35
…karound

tests/entity_mapping/test_entity_hierarchy.py imported from
presidio_evaluator.entity_mapping.hierarchy which does not exist (the
module lives in mapper.py and is re-exported via the package __init__).
Updated import to use presidio_evaluator.entity_mapping directly.

.pre-commit-config.yaml: remove --ignore=tests/entity_mapping/test_entity_hierarchy.py
now that the file imports correctly and all 145 tests pass.
…ls=) in notebooks 4/5/6 and clean up stale comments
- Fix 5 integration tests that used deprecated TokenEvaluator(model=...) pattern
- Update test_notebook.py to use PresidioAnalyzerWrapper + CanonicalMapper + TokenEvaluator(model=None)
- Fix test_full_pipeline_integration SpanEvaluator and TokenEvaluator to use model=None
- Update test_presidio_analyzer_wrapper.py to use model=None in TokenEvaluator
- Update test_recognizers_generated_text.py to use model=None, entities_to_keep=model.entities
- Update test_recognizers_template_csv.py to use model=None, entities_to_keep=model.entities
- Fix notebooks 4 and 5: add global PII metrics pass (per_type=False) to compute
  pii_f, pii_precision, pii_recall (were None because only per_type=True was called)
- Notebook 4 verified: PII F=0.66, precision=0.73, recall=0.65 (all > 0.5)
compare(), __revert_known_errors(), _adjust_per_entities(), and
evaluate_sample() are token-level concerns only used by TokenEvaluator.
Move them out of BaseEvaluator and into TokenEvaluator where they belong.

- BaseEvaluator: remove compare, __revert_known_errors, _adjust_per_entities,
  evaluate_sample; inline _adjust_per_entities into deprecated get_results_dataframe;
  remove now-unused spacy Token import
- TokenEvaluator: add all four methods; add logging + spacy Token + ErrorType + ModelError imports
- MockEvaluator in test_evaluator.py: inherit from TokenEvaluator (was BaseEvaluator)
- Fix stale Evaluator(, ...) patterns across test files and docs (add model=None)
…h modes

- SpanEvaluator: rename inner method to _run_score_pass(per_type, ...),
  add public calculate_score_on_df(results_df, level='both', ...) that
  dispatches to the two passes based on level
- TokenEvaluator: add level param to calculate_score_on_df and
  calculate_score; conditionally compute entity or PII metrics
- Update all callers (test_span_evaluator, test_plotter, test_notebook)
  to use level='entity' / level='pii' / level='both'
- Update notebooks 4, 5 (two-call pattern → single call with default
  level='both') and notebook 6 (per_type=True → level='entity')
- Update base_evaluator.py docstrings
@omri374 omri374 changed the title Presidio evaluator redesign Presidio evaluator redesign towards v0.3 Mar 26, 2026
…display_mode=none to Plotter; extract test fixtures; fix ruff linting
@omri374
omri374 requested a review from Copilot March 26, 2026 23:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR redesigns Presidio Evaluator around a decoupled three-layer pipeline (Model → CanonicalMapper → Evaluator) and modernizes tooling (uv, ruff, pre-commit) while removing non-Presidio model wrappers.

Changes:

  • Introduces BaseModel.predict_dataset() as the primary inference output (5-column results DataFrame) and shifts scoring to calculate_score_on_df().
  • Adds a new entity_mapping package with CanonicalMapper/EntityHierarchy and regression tests for label coverage.
  • Migrates packaging/tooling from Poetry to PEP 621 + Hatch/uv, adds Ruff config, pre-commit, and updates CI.

Reviewed changes

Copilot reviewed 103 out of 121 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
presidio_evaluator/models/base_model.py Adds predict_dataset() DataFrame contract and removes model-owned entity mapping.
tests/integration/conftest.py Adds automatic integration marker assignment for integration tests.
.github/workflows/ci.yml Switches CI to uv and updates Python matrix.
pyproject.toml Migrates from Poetry to PEP 621, updates dependency constraints and Python requirement.
ruff.toml Introduces Ruff lint/format configuration for the repo.
Comments suppressed due to low confidence (1)

presidio_evaluator/models/presidio_analyzer_wrapper.py:47

  • BaseModel.__init__ now hard-errors if entity_mapping is provided, but PresidioAnalyzerWrapper still exposes an entity_mapping parameter and forwards it to BaseModel. This creates a confusing public API (parameter exists but is effectively invalid). Remove the entity_mapping parameter from PresidioAnalyzerWrapper.__init__ (and other wrappers if applicable), or keep it but raise a dedicated, wrapper-level DeprecationError with migration instructions.
        super().__init__(
            entities_to_keep=entities_to_keep,
            verbose=verbose,
            labeling_scheme=labeling_scheme,
            entity_mapping=entity_mapping,
        )

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/integration/conftest.py
Comment thread presidio_evaluator/models/base_model.py
Comment thread presidio_evaluator/models/base_model.py
Comment thread presidio_evaluator/models/presidio_analyzer_wrapper.py
Comment thread presidio_evaluator/models/presidio_analyzer_wrapper.py
Comment thread .github/workflows/ci.yml Outdated
omri374 and others added 2 commits March 28, 2026 18:30

- **`predict_dataset` materializes all predictions in memory** — for very large datasets, the full DataFrame is held in RAM. The current `evaluate_all()` loop can in principle be streamed (though it isn't today).
- **`evaluate_all()` becomes a thin wrapper** — code that currently calls `evaluate_all()` and inspects `List[EvaluationResult]` directly will need to be updated if it relies on the per-sample `EvaluationResult` structure. Backward compatibility is preserved at the call site, but the internal representation changes.
- **Schema enforcement is implicit** — the 5-column schema is a convention, not enforced by a type. Callers that hand-construct DataFrames must respect column names and types.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we validate the dataframe schema?

self._records: dict[str, _Resolution] = {}
self._auto_resolve()

def get_mapped_results_dataframe(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user calls get_mapped_results_dataframe after defining some custom entities, wouldn't this wipe out these user defined entities? For example, if we get warning regarding the different hierarchy levels, and get_mapped_results_dataframe is called again to resolve the conflict, self._records.clear() will remove all existing mappings including the manually defined ones


# ── Mutation ─────────────────────────────────────────────────────────────

def map(self, mappings: dict[str, str | None]) -> CanonicalMapper:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should manual entities be exported for future use/reproducibility? And also, should we make sure the manual entities are consistent across models?

@omri374

omri374 commented Apr 14, 2026

Copy link
Copy Markdown
Collaborator Author

@negruber1 thanks! great points. I'll release a new ADR soon with the things we mentioned yesterday, and after we agree on this will update the code including with your suggestions here.

@omri374

omri374 commented Apr 14, 2026

Copy link
Copy Markdown
Collaborator Author

@negruber1 something I thought about: If we look at all the collisions between annotated and predicted entities to surface what needs to be fixed, are we adding a confounding effect to the comparison in any way? Could be minor/negligible though.

@omri374
omri374 marked this pull request as draft April 14, 2026 18:51
@negruber1

negruber1 commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

@omri374 I think that if the conflicts are resolved by the user BEFORE seeing evaluation results, the confounding effect is probably minor: If there is a "discovery trial" where you get to see the model's vocabulary without calculating the evaluation metrics, the user decides to resolve based on the existing labels and not which labels the model gets wrong (We can even recommend that on the mapper README). We can always evaluate at all hierarchies and make this available for the user regardless of what mappings they decided to define manually.
I think that because we include human input, we can't avoid a confounding effect entirely.

@omri374

omri374 commented Apr 15, 2026

Copy link
Copy Markdown
Collaborator Author

@omri374 I think that if the conflicts are resolved by the user BEFORE seeing evaluation results, the confounding effect is probably minor: If there is a "discovery trial" where you get to see the model's vocabulary without calculating the evaluation metrics, the user decides to resolve based on the existing labels and not which labels the model gets wrong (We can even recommend that on the mapper README). We can always evaluate at all hierarchies and make this available for the user regardless of what mappings they decided to define manually. I think that because we include human input, we can't avoid a confounding effect entirely.

Thanks, completely agree.

@omri374

omri374 commented Apr 17, 2026

Copy link
Copy Markdown
Collaborator Author

closing this PR in favor of an updated implementation

@omri374 omri374 closed this Apr 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants