fix(analyzer): make the shipped HuggingFaceNerRecognizer entry loadable - #2227
Open
Aryan-Pardeshi wants to merge 2 commits into
Open
Conversation
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
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.
Fixes #2222
HuggingFaceNerRecognizerships indefault_recognizers.yamlwithenabled: falseand nomodel_name. Flipping that switch does not work:EntityRecognizer.__init__callsload(),load()raised whenmodel_namewas unset, and the error therefore arrived during registry construction rather than at analyze time. The shipped entry advertised a toggle that always crashed.The issue offers three acceptable fixes. I took the deferred-load one:
load()now returns early with an info-level log whenmodel_nameis unset, so the registry builds.analyze()raises aValueErrornaming what is missing and where to set it, includingdefault_recognizers.yaml, so the failure surfaces at the point of use with an actionable message.I deliberately did not give the shipped entry a default
model_name. Committing the project to a specific HuggingFace model reference in shipped config is a product decision rather than a bug fix, and it would make the loader test download a model. Happy to switch to that if you would rather — it is a small change on top of this one.Because the entry now loads,
HuggingFaceNerRecognizerhas been removed from theNOT_LOADABLE_FROM_SHIPPED_ENTRYexclusion intest_recognizers_loader_utils.py, which #2170 added with a guard test to keep the gap visible. That set is now empty, and the recognizer is covered by the normal load test like every other entry. I left the comment in place explaining why it is empty, so the next person to need it knows what it was for.The existing test asserting the old
ValueErrorfromload()is updated to assert the new analyze-time error instead, and a test covers registry construction succeeding with the recognizer instantiated and unloaded.presidio-analyzer/tests/test_huggingface_ner_recognizer.pyandtest_recognizers_loader_utils.py: 338 passed, 3 skipped.ruff check presidio_analyzer testsreports all checks passed.