feat(detection): add regex entity detection - #265
Conversation
Signed-off-by: lipikaramaswamy <lramaswamy@nvidia.com>
Greptile SummaryThis PR adds regex-based entity detection as a first-class source alongside GLiNER and LLM detection.
Confidence Score: 5/5The implementation appears safe to merge, with one non-blocking publication-metadata inconsistency in the bundled skill card. The two previous behavioral findings are fully fixed: route recombination now preserves source precedence, and URL trimming retains balanced delimiters. The only new issue is a non-blocking inconsistency where the skill card claims readiness while the benchmark says required evaluation evidence is missing. Files Needing Attention: skills/anonymizer/skill-card.md Important Files Changed
|
| ) | ||
| expanded = expand_entity_occurrences(text=text, entities=validated) | ||
| accepted_regex = _parse_entity_spans(row.get(COL_REGEX_ACCEPTED_ENTITIES, {})) | ||
| protected = merge_entity_sources(accepted_regex, validated) |
There was a problem hiding this comment.
When an overlapping built-in rule bypasses LLM validation while a higher-priority user rule takes the default LLM route, this final merge always prioritizes the accepted-route list. For identical spans with different labels, it therefore discards the validated user match in favor of the built-in match, violating the documented regex_user > regex_builtin > detector precedence and producing the wrong final label. Merge the routes using each candidate's source priority instead of argument order.
There was a problem hiding this comment.
Fixed in 46907f8. Validation routes are now recombined by candidate provenance, preserving regex_user > regex_builtin > other detector sources regardless of validate_with_llm. Regression tests cover both route permutations and the finalization merge.
| if rule.label == "url": | ||
| trimmed = text[start:end].rstrip(_URL_TRAILING_PUNCTUATION) | ||
| end = start + len(trimmed) |
There was a problem hiding this comment.
Blindly stripping every trailing closing delimiter truncates valid URLs containing balanced parentheses, such as https://en.wikipedia.org/wiki/Foo_(bar). The shortened value still passes URL validation and is emitted with an end offset before the legitimate ), so replacement operates on a malformed partial URL. Make punctuation trimming balance-aware rather than applying rstrip unconditionally.
There was a problem hiding this comment.
Fixed in 46907f8. URL suffix trimming is now delimiter-balance-aware: balanced delimiters remain part of the URL, while unmatched prose closers are removed. Tests cover nested ASCII delimiters, CJK pairs, unmatched closers, IPv6 host brackets, and exact offsets.
Signed-off-by: lipikaramaswamy <lramaswamy@nvidia.com>
|
/nvskills-ci |
Signed-off-by: nvskills-svc-account <svc-nvskills-signing@nvidia.com>
| This skill package is prepared for NVSkills publication review. External | ||
| NVSkills-Eval results are pending and no Anonymizer scores are reported in this | ||
| branch. | ||
| This skill is ready for commercial/non-commercial use. <br> |
There was a problem hiding this comment.
The benchmark says required Tier 2 evidence was not produced and the evaluation is not publication-complete, but this skill card declares the skill ready for commercial and non-commercial use without that qualification. Readers relying on the card could mistake an incompletely evaluated publication candidate for a ready artifact. Please align the readiness claim with the benchmark or disclose the missing evaluation evidence.
| rule_id="nemo.email.v1", | ||
| label="email", | ||
| pattern=( | ||
| r"(?<![A-Za-z0-9.!#$%&'*+/=?^_`{|}~-])" |
There was a problem hiding this comment.
Some valid international email domains are missed because these character classes exclude Unicode combining marks. I reproduced this with x@उदाहरण.भारत and decomposed x@éxample.com; both domains encode successfully with IDNA but produce no match here. Could we allow IDNA-valid marks, possibly after normalization, and add a non-CJK IDN test?
| ResolvedRegexRule( | ||
| rule_id="nemo.ipv6.v1", | ||
| label="ipv6", | ||
| pattern=r"(?<![0-9A-Fa-f:])(?:[0-9A-Fa-f]{0,4}:){2,7}[0-9A-Fa-f]{0,4}(?![0-9A-Fa-f:])", |
There was a problem hiding this comment.
IPv4-embedded IPv6 addresses get split unexpectedly here. For example, ::ffff:192.0.2.128 is detected as ::ffff:192, which IPv6Address accepts as a different valid address. With IPv4 detection enabled, the dotted portion may be replaced while ::ffff: remains visible. Could we extend the pattern to consume dotted IPv4 tails and add an exact-span test?
| if isinstance(validator, str): | ||
| _resolve_validator(validator) | ||
| return validator | ||
| module = getattr(validator, "__module__", "") |
There was a problem hiding this comment.
One edge case with the local validator registry: two closures returned by the same factory share the same module and qualified name, so registering the second raises a duplicate-registration error even though it is a different callable. An opaque identity-based key for direct callables would avoid the collision while keeping stable names for explicit entry-point validators.
| compiled = regex.compile(value) | ||
| except regex.error as exc: | ||
| raise ValueError(f"Invalid regex pattern {value!r}: {exc}") from exc | ||
| match = compiled.search("") |
There was a problem hiding this comment.
I think contextual zero-width rules can slip through this check. Patterns such as (?=CASE) and (?<=A) construct successfully, but detection later skips all their zero-length matches, so the rule silently does nothing. It would be safer to reject these during configuration validation, with a couple of lookahead and lookbehind tests.
|
|
||
|
|
||
| def _validate_url(candidate: RegexCandidate) -> bool: | ||
| target = candidate.value if not candidate.value.startswith("www.") else f"https://{candidate.value}" |
There was a problem hiding this comment.
Small case-sensitivity mismatch here: the regex accepts WWW.example.com, but this prefix check only recognizes lowercase www.. That leaves the value without a scheme, and urlsplit rejects it. A case-insensitive check such as candidate.value.lower().startswith("www.") should cover it.
| @@ -1,85 +1,125 @@ | |||
| <!-- SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. --> | |||
| <!-- SPDX-License-Identifier: Apache-2.0 --> | |||
| # Skill Benchmark: anonymizer | |||
There was a problem hiding this comment.
The copyright check currently flags this file and skills/anonymizer/skill-card.md because they are missing the repository's SPDX headers. Adding the standard headers to both should clear the aggregate CI failure.
| [issue #262](https://github.com/NVIDIA-NeMo/Anonymizer/issues/262). This | ||
| document scopes the deterministic regex and validator feature as a focused | ||
| slice of the broader | ||
| [Multi-Pole Detection](../detection-poles/multi-pole-detection.md) design. |
There was a problem hiding this comment.
This link points to plans/detection-poles/multi-pole-detection.md, which isn't present in the PR. Since the broader framework is larger than this change, could we replace it with a follow-up issue or design doc? A short scope covering common detector output, configurable fan-in, and execution/failure contracts would make the intended direction clear without expanding this implementation.
|
|
||
| Avoid logging raw entity values in aggregate telemetry. | ||
|
|
||
| The source presentation asks whether deterministic and model detection can run |
There was a problem hiding this comment.
Data Designer 0.9.1 already schedules independent graph columns concurrently, so the regex and GLiNER branches should overlap today. I think this paragraph can describe parallel poles as part of the current graph contract and leave the shared resource, timeout, cancellation, and failure model for the follow-up design.
|
|
||
| Merge policy: | ||
|
|
||
| 1. Reject malformed or out-of-bounds spans. |
There was a problem hiding this comment.
One edge case to consider before resolving overlaps here: the winning candidate hasn't necessarily been contextually validated yet. A longer candidate can remove an overlapping fallback, then get dropped by the LLM, leaving neither candidate. Coalescing only exact same-label/span matches before validation, while retaining all their origins, would let final overlap resolution happen after the decisions are known.
| ## Workflow Architecture | ||
|
|
||
| The current seed path parses GLiNER output directly into `COL_SEED_ENTITIES`. | ||
| Split candidate generation from seed fan-in: |
There was a problem hiding this comment.
This feels like the right extension seam. It may be worth noting that GLiNER and regex are the first two producers of a future common detector-output contract, with fan-in eventually accepting a configured set of producer columns. The current source-specific implementation can stay, but the note would help keep the next detector from adding another parallel set of constants and merge paths.
|
The overall direction makes sense, and regex plus GLiNER as independent Data Designer columns feels like a solid first step toward layered detection. I don't think this PR needs to build the full multi-detector framework. Keeping the public regex API and focusing here on correct two-source behavior seems like the right scope. Could we capture a follow-up issue or design for the general framework? I'd include a shared candidate/outcome shape with multiple origins, one independently schedulable column per detector, generic fan-in, overlap resolution after validation, and per-detector preparation, resource, timeout, and failure contracts. A useful acceptance test would be adding a dummy third detector without changing fan-in, while producing identical results regardless of completion order. That follow-up could also replace the currently missing multi-pole design referenced by this plan. |
Related Issue
Fixes #262.
Plan Document
Hybrid regex entity detection plan
Summary
Adds regex-based entity detection as a first-class detection source alongside GLiNER and LLM detection.
Detect.regex_rulesconfiguration surface for built-in customization and user-defined rules.Type of Change
Contributor Checklist
type(scope): description).Validation
TMPDIR=/tmp make test— 1312 passed, 1 warning.make format-check— passed.ty checkfor changed files — passed.make docs-buildfrom a clean worktree at this commit — passed.Documentation and Artifacts
make docs-buildpasses locally.