fix(analyzer): make every recognizer listed in default_recognizers.yaml loadable

[developer0hye]

Change Description

RecognizerListLoader.get instantiates every predefined recognizer with a name keyword argument, taken from the YAML entry — or from name when class_name supplies the class, which is the documented rename path (see RecognizerListLoader.get_recognizer_name). A recognizer whose __init__ does not accept name therefore cannot be built from a registry configuration at all. Four recognizers were in that state.

Three are listed in default_recognizers.yaml with enabled: false and cannot be turned on. KrBrnRecognizer, KrDriverLicenseRecognizer and UsMbiRecognizer raise TypeError: __init__() got an unexpected keyword argument 'name' before the registry finishes loading. enabled: false is an opt-in switch, not a disclaimer — an entry that cannot be enabled should not be listed. The failure also reads as a user configuration error even though the YAML is correct, unlike the optional-dependency entries, which refuse to load with an actionable ImportError naming what to install.

One is absent from the YAML because of the same defect. KrPassportRecognizer was the only Korean recognizer missing from default_recognizers.yaml; listing it would have made the registry raise. It also defaulted to supported_language="kr" while every other Korean recognizer defaults to ko (ISO 639-1). #1742 migrated the Korean recognizers from kr to ko; #1814 added this one afterwards and reintroduced kr, so an AnalyzerEngine running ko silently skipped it. That half only affects direct instantiation — when an entry omits supported_languages the loader passes the registry's language explicitly, so the class default never applies on the YAML path.

Measured across the whole shipped configuration, every entry forced to enabled: true and loaded:

yaml entries:                  86
shipped with enabled: false:   60
load fine when enabled:        81
FAIL when enabled:              5

Two of the five are expected (HuggingFaceNerRecognizer, BasicLangExtractRecognizer — actionable ImportError for an optional dependency). The other three are the TypeError.

Changes

• Added the name argument, forwarded to super().__init__, on KrPassportRecognizer, KrBrnRecognizer, KrDriverLicenseRecognizer and UsMbiRecognizer. Appended last in each signature, so existing positional callers are unaffected.
• Changed KrPassportRecognizer's default supported_language to ko.
• Registered KrPassportRecognizer in default_recognizers.yaml with enabled: false and country_code: kr, matching its siblings.
• Added tests/test_predefined_recognizer_contract.py.

Why a contract test, and why the existing ones missed this

The constructor signature is part of a contract nothing enforced, and the existing tests miss it from both sides:

• Each recognizer's own unit tests instantiate the class directly, where no name is passed. They all pass.
• The registry-level contract tests — test_predefined_pattern_recognizers_have_the_right_regex_flags, test_default_recognizers_yaml_country_code_matches_class — build the default configuration, in which ~60 entries are disabled and therefore never constructed.

The new tests close the gap from both directions:

• Signature. Every predefined PatternRecognizer subclass must accept the kwargs the loader passes. This fires when a class is added, before it reaches the YAML at all — the point at which KrPassportRecognizer went wrong in feat: Add Korean passport number recognizer (KR_PASSPORT) #1814.
• Load. Every entry in default_recognizers.yaml must resolve to a class and must load once enabled, exercised entry by entry so a failure names the recognizer. Entries gated behind an optional dependency are skipped, since refusing to load with an actionable ImportError is their intended behavior.
• Intent. A class_name + name entry must produce an instance with the configured name. This is the documented reason the loader passes name, so it records why the kwarg contract is load-bearing rather than incidental.

Scope of the load test

The load test covers non-pattern entries too, deliberately. Narrowing it to PatternRecognizer subclasses would silently drop PhoneRecognizer, ZaMobileNumberRecognizer and ZaTelephoneNumberRecognizer, which are not PatternRecognizers and do load.

One entry is excluded by name, with its reason recorded next to the exclusion. A further test asserts every exclusion still matches a shipped entry, so the list cannot rot if an entry is renamed, removed, or fixed:

• HuggingFaceNerRecognizer cannot load from its shipped entry even with its dependencies installed. EntityRecognizer.__init__ calls load() unconditionally, load() requires model_name, and the entry does not supply one — so with transformers and torch present it raises ValueError: model_name must be set before calling load(), not the ImportError it raises when they are absent. Supplying model_name in the YAML would make the test download a model. The entry stays covered by the resolve test. This is a pre-existing defect in the entry (HuggingFaceNerRecognizer is not usable from the shipped configuration on main either); worth a separate issue.

BasicLangExtractRecognizer is skipped only on ImportError, since refusing to load without the langextract extra is that recognizer's intended behavior. The skip is scoped to that name rather than to the exception type, so an ImportError from any other entry is a failure instead of a green skip.

That recognizer also carries a config_path that it resolves against the working directory, so it raises FileNotFoundError when pytest runs from the repository root. The load test sets the working directory to the component root — the same one CI uses — instead of catching that error, because catching FileNotFoundError would turn a deleted or renamed shipped config file into a passing skip. A separate test asserts every config_path in the shipped configuration resolves to a file that exists, which holds even in an environment that cannot construct the recognizer at all. (The more robust YAML value would be config_path: langextract_config_basic.yaml, which resolve_config_path finds from any cwd — out of scope here.)

The signature test is scoped to PatternRecognizer subclasses. AzureAILanguageRecognizer is the one remaining class that does not accept name; it is absent from default_recognizers.yaml and fixes its own display name, so it is out of scope here — but it is reachable from a user config by class name and fails the same way. Pre-existing; worth tracking separately.

Behavior change

The kr → ko default is a breaking change for direct instantiation. KrPassportRecognizer() now registers under ko, so a caller analysing with language="kr" no longer gets KR_PASSPORT results. The failure is silent, not an exception: the registry raises only when the whole result set is empty, and the per-entity path just logs Entity KR_PASSPORT doesn't have the corresponding recognizer in language : kr. Migration is one line — pass supported_language="kr", or switch the call to language="ko".

Registry/YAML users are unaffected: the loader always passes supported_language explicitly, and the new entry declares both ko and kr. No call site in this repository passes language="kr", and no shipped NLP configuration declares kr.

Verification

• Reverting the three constructors makes the signature test and the load test each fail for exactly those three, with no other failures — the tests catch the defect they were written for.
• Full presidio-analyzer suite run on this branch and on an origin/main worktree: the failure sets are identical (42 pre-existing failures from optional dependencies absent in the local environment — transformers, langextract, some spaCy models). No regressions.
• The new module was also run with the transformers/torch module globals patched to non-None, which reproduces a full-extras CI environment without installing them: 258 passed, 1 skipped, no failures. An earlier revision of this branch failed there, which is what the HuggingFaceNerRecognizer exclusion above fixes.
• Injecting an ImportError into a non-excluded entry's constructor makes the load test fail rather than skip, confirming the narrowed exception scope.
• Pointing a config_path at a name that does not exist makes the new config-path test fail, confirming a deleted or renamed shipped file is caught rather than skipped.
• The module passes identically when run from presidio-analyzer and from the repository root (258 passed, 1 skipped both ways), so it is not working-directory dependent.
• ruff check from the repo root (as CI runs it), ruff format --check on the modified files, and git diff --check all pass.
• The default_recognizers.yaml diff is an 8-line pure addition.

Per the changelog policy in #2200, CHANGELOG.md is not modified — the earlier revision of this branch edited it, which is what made the PR conflict.

Issue reference

Fixes #2176. Follows up on #1742 (Korean kr → ko migration) and #1814 (which added KrPassportRecognizer).

Checklist

• I have reviewed the contribution guidelines
• I agree to follow this project's Code of Conduct
• I confirm that I have the right to submit this contribution and that it does not knowingly contain proprietary or confidential code.
• My code includes unit tests
• All unit tests and lint checks pass locally
• My PR contains documentation updates / additions if required

🤖 Generated with Claude Code

[Copilot]

🟡 Not ready to approve

The updated KR passport test adds avoidable debug output and uses tempfile.mkdtemp() without guaranteed cleanup, which should be corrected before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR strengthens the Presidio Analyzer predefined recognizer loading contract by ensuring YAML-configured recognizers are instantiable (notably when enabled: false entries are toggled on), and adds contract tests to prevent regressions.

Changes:

• Updated several predefined recognizers to accept the name kwarg (as passed by RecognizerListLoader) and forward it to super().__init__.
• Corrected KrPassportRecognizer’s default supported_language to ko and registered it in default_recognizers.yaml (disabled by default).
• Added contract tests to validate (a) recognizer constructor signature compatibility and (b) that each shipped YAML entry resolves and loads when enabled.

File summaries

File	Description
presidio-analyzer/tests/test_predefined_recognizer_contract.py	Adds contract tests for loader/recognizer constructor compatibility and for shipped YAML entry loadability.
presidio-analyzer/tests/test_kr_passport_recognizer.py	Extends KR passport tests to cover default language, name kwarg acceptance, and loading from the shipped YAML.
presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/us/us_mbi_recognizer.py	Makes UsMbiRecognizer loadable from YAML by accepting and forwarding name.
presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/korea/kr_passport_recognizer.py	Fixes default language (kr → ko) and adds name parameter forwarding for YAML instantiation.
presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/korea/kr_driver_license_recognizer.py	Accepts and forwards name to support YAML-driven instantiation.
presidio-analyzer/presidio_analyzer/predefined_recognizers/country_specific/korea/kr_brn_recognizer.py	Accepts and forwards name to support YAML-driven instantiation.
presidio-analyzer/presidio_analyzer/conf/default_recognizers.yaml	Registers KrPassportRecognizer (disabled) with country/language metadata.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[omri374]

Thanks! Looks great, left a minor comment on tests.

[juno-junho]

I filed #2217 / #2218 for the same name-kwarg crash before finding this PR, sorry for the duplicate. I've rescoped my PR to only the parts yours doesn't cover (three other recognizers missing from the yaml), so the two are complementary now.

One small suggestion while I was in this area: a signature-level regression test keeps future yaml entries honest without instantiating anything (instantiation sweeps don't work here because ML entries like HuggingFaceNerRecognizer legitimately require configuration before they can be built). Something like:

def test_when_default_yaml_lists_a_predefined_recognizer_then_its_constructor_accepts_name():
    conf = yaml.safe_load(DEFAULT_RECOGNIZERS_YAML.read_text())
    rejecting = []
    for recognizer_conf in conf["recognizers"]:
        if recognizer_conf.get("type") != "predefined":
            continue
        cls = getattr(predefined, recognizer_conf.get("class_name", recognizer_conf["name"]))
        params = inspect.signature(cls.__init__).parameters
        accepts_kwargs = any(p.kind is p.VAR_KEYWORD for p in params.values())
        if "name" not in params and not accepts_kwargs:
            rejecting.append(recognizer_conf["name"])
    assert not rejecting

Before your fix it names exactly the four offenders. Feel free to take it or ignore it, happy either way. Thanks for fixing this.

[developer0hye]

No apology needed — and thanks for rescoping #2218 rather than dropping it. AbaRoutingRecognizer, FiPersonalIdentityCodeRecognizer and SgUenRecognizer are outside this PR, they're all PatternRecognizers that already accept name, and the two branches only meet on one default_recognizers.yaml hunk. Once both land, the load test here parametrizes over your three new entries automatically, so they get the enable-safe check without you wiring anything up.

Took the suggestion, in 7304d492 — but narrowed to the part that was actually uncovered, because measuring it turned up less overlap than either of us assumed in one direction and more in the other.

Where you were right. My signature sweep was package-driven and scoped to PatternRecognizer subclasses, which left five shipped entries unreached: PhoneRecognizer, the two Za* ones, and the NER/LLM wrappers. Three of those the load test constructs outright, so they were covered more strongly than a signature check anyway. But HuggingFaceNerRecognizer is excluded from the load test — its shipped entry supplies no model_name, so EntityRecognizer.__init__ calling load() raises ValueError once transformers is present — which means nothing in the suite touched its constructor at all. That's the hole, and it's the one your framing points straight at: a check that builds nothing keeps working where a sweep that builds things has to give up.

So the test now parametrizes over the union of both sources. Neither subsumes the other, which is the part worth recording:

• package-driven reaches a class before it is listed anywhere;
• yaml-driven reaches a listed class the package sweep skips by base class.

One correction, and it's the reason the sweep starts from the package. A yaml-driven check names three offenders on main, not four. KrPassportRecognizer isn't in main's default_recognizers.yaml — I checked — and that isn't incidental: it's absent because its constructor rejected name, so listing it would have made the registry raise. The class that most needed catching was the one structurally invisible to a yaml-driven test. That's the #1814 failure mode, and it's why the package sweep stays.

And one thing that didn't work out the way the snippet implies. HuggingFaceNerRecognizer.__init__ takes **kwargs, so accepts_kwargs exempts it — your version passes it without asserting anything, and so does mine. No signature test can do better here; a **kwargs constructor simply doesn't say which kwargs it honors. Mine reports it as a named skip rather than a silent pass, so the gap stays visible in the run:

SKIPPED HuggingFaceNerRecognizer.__init__ takes **kwargs, so the signature cannot
        show which kwargs it honors. Reported as a skip rather than a silent pass
        so the gap in coverage stays visible.

Net effect: 5 new params — 3 assertions, 2 skips (HuggingFaceNerRecognizer, BasicLangExtractRecognizer, both **kwargs). 297 passed, 3 skipped, up from 294 / 1. Renamed to test_recognizer_accepts_loader_kwargs since it's no longer pattern-only.

HuggingFaceNerRecognizer being unusable from its shipped entry is a real pre-existing defect, separate from this PR and from yours — it's recorded in NOT_LOADABLE_FROM_SHIPPED_ENTRY with a guard test asserting the exclusion still names a shipped entry, so it can't rot into silently narrowed coverage. Worth its own issue; say the word if you'd rather file it, since you found the same edge from the other side.

[juno-junho]

The 3-vs-4 mismatch is on me: I counted on my own branch, which already had the KrPassport yaml entry added, so the yaml-driven check saw four there. On main it's three, as you said. And your point lands: the class that most needed catching is exactly the one a yaml-driven check is structurally blind to, which argues for the package sweep better than anything in my snippet.

The union parametrization plus the named skip is strictly better than what I posted. Thanks for measuring it instead of taking it as-is; the **kwargs limitation in particular is something I hadn't thought through.

I'll file the HuggingFaceNerRecognizer issue. I hit the same edge from the other direction (an enable-everything sweep while benchmarking #2216) and have the repro handy, so it's a natural fit. I'll reference your NOT_LOADABLE_FROM_SHIPPED_ENTRY guard so the two ends stay linked.

[developer0hye]

Thanks — and the miscount was a useful one to have made. Counting on a branch that already had the entry is exactly the situation the test will be in from here on, so it surfaced the asymmetry faster than counting on main would have.

Glad the issue is going to you. A repro from an enable-everything sweep is better evidence than the exclusion constant on its own, since it shows the entry failing the way a user would hit it rather than the way a test avoids it. I'll link the issue from NOT_LOADABLE_FROM_SHIPPED_ENTRY once it's up, so the constant points at the reason it exists instead of just naming the class.

Looking forward to #2218 landing.

[juno-junho]

It's up: #2222. Thanks again.