fix(analyzer): honour language_model_params in BasicLangExtractRecogn…#2150
Merged
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
SharonHart
previously approved these changes
Jul 12, 2026
RonShakutai
force-pushed
the
ronshaktuai/fix-langextract-params
branch
from
953478f to
6cea8bd
Compare
6 tasks
SharonHart
approved these changes
Jul 13, 2026
Contributor
Coverage report (presidio-analyzer)
Click to see where and how coverage changed
The report is truncated to 25 files out of 70. To see the full report, please visit the workflow summary page. This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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)