fix(analyzer): honour language_model_params in BasicLangExtractRecogn…#2150
Open
RonShakutai wants to merge 2 commits into
Open
fix(analyzer): honour language_model_params in BasicLangExtractRecogn…#2150RonShakutai wants to merge 2 commits into
RonShakutai wants to merge 2 commits into
Conversation
…izer Fixes #1942. Merge language_model_params into provider_kwargs so timeout, num_ctx and other LLM provider params configured in YAML actually reach the provider (e.g. Ollama) instead of being silently dropped. Also fixes TypeError when kwargs: or language_model_params: is null in YAML. - Merge language_model_params into provider_kwargs via setdefault (so explicit kwargs: entries still win for backwards compatibility) - Guard against null YAML values with 'or {}' - Strengthen regression tests to assert params reach ModelConfig.provider_kwargs - Add test for kwargs: null edge case - Document fix in CHANGELOG Related: #1943 (lsternlicht/fix-basic-langextract-language-model-params-dropped)
Contributor
Coverage report (presidio-anonymizer)
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
Contributor
Coverage report (presidio-structured)
This PR does not seem to contain any modification to coverable code. |
Contributor
Coverage report (presidio-image-redactor)
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
Contributor
Coverage report (presidio-cli)
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a configuration propagation bug in the analyzer’s BasicLangExtractRecognizer, ensuring that YAML-defined langextract.model.provider.language_model_params (e.g., timeout, num_ctx) actually reach the LangExtract provider when a pre-built ModelConfig is passed.
Changes:
- Merge
provider.language_model_paramsintoModelConfig.provider_kwargsusingsetdefault()so explicitprovider.kwargskeeps precedence. - Harden YAML parsing for
kwargs: null,extract_params: null, andlanguage_model_params: nullby treating them as empty dicts. - Add/strengthen regression tests to assert that parameters land on
ModelConfig.provider_kwargs(not only on thelanguage_model_paramsargument tolangextract.extract()), plus an explicitkwargs: nulledge-case test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| presidio-analyzer/presidio_analyzer/predefined_recognizers/third_party/basic_langextract_recognizer.py | Ensures language_model_params are applied by merging into provider_kwargs used to build ModelConfig, and guards against null YAML values. |
| presidio-analyzer/tests/test_basic_langextract_recognizer.py | Adds regression and backwards-compat tests verifying params reach ModelConfig.provider_kwargs, including a kwargs: null YAML edge case. |
| CHANGELOG.md | Documents the analyzer fix and the null-YAML TypeError fix. |
6 tasks
omri374
previously approved these changes
Jul 9, 2026
6 tasks
… registry (#2153) LM recognizers (BasicLangExtractRecognizer, AzureOpenAILangExtractRecognizer) configured via a recognizer registry YAML silently ignored config_path: the strict PredefinedRecognizerConfig schema has no config_path field and forbids extras, so Pydantic dropped it and the recognizer fell back to its bundled default model configuration. Add a LangExtractRecognizerConfig model (extra=allow, explicit config_path field) mirroring the existing HuggingFace/GLiNER configs, and register both LM recognizer class names in CONFIG_MODEL_MAP so config_path (and other recognizer-specific kwargs) survive validation and reach the constructor. Adds regression tests covering config_path preservation via both the model and the full ConfigurationValidator registry path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SharonHart
approved these changes
Jul 12, 2026
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.
Continues and completes PR #1943 started by @lsternlicht
Fixes #1942
Summary
BasicLangExtractRecognizerwas silently droppinglanguage_model_params(like
timeout,num_ctx) configured in YAML becauselangextract.extract()ignores its
language_model_paramsargument when a pre-builtModelConfigispassed via
config=. This fix mergeslanguage_model_paramsintoprovider_kwargsso they reach the provider (e.g., Ollama) at runtime.
This PR builds on the original work from @lsternlicht and incorporates all
feedback from code review (Copilot, @RonShakutai), addressing edge cases and
adding comprehensive test coverage.
Changes
language_model_paramsintoprovider_kwargsviasetdefault()(explicit
kwargs:entries still take precedence for backwards compatibility)nullYAML values withor {}safeguardModelConfig.provider_kwargskwargs: nulledge case (per @RonShakutai feedback)Validation
✅ All 22 tests pass
✅ Custom timeout reaches Ollama HTTP call
✅ num_ctx applied in langextract.extract()
✅ max_char_buffer passed correctly
✅ Edge case: kwargs: null in YAML doesn't crash
Addresses all feedback from PR #1943 (@lsternlicht, @copilot, @RonShakutai)