Add a configurable LangExtract recognizer for use with any provider. #1815
Conversation
…with any provider.
…with any provider.
RonShakutai
left a comment
RonShakutai
left a comment
There was a problem hiding this comment.
What a great PR !
he lx_factory.ModelConfig approach is elegant !
left a few comments to finalize it
| - en | ||
| type: predefined | ||
| enabled: false | ||
| config_path: presidio-analyzer/presidio_analyzer/conf/langextract_config_ollama.yaml |
There was a problem hiding this comment.
Can we remove OllamaLangExtractRecognizer?
BasicLangExtractRecognizer already supports Ollama through provider configuration.
The dedicated Ollama recognizer seems redundant now.
also should we adjust the e2e tests as well https://github.com/microsoft/presidio/blob/main/e2e-tests/tests/test_package_e2e_integration_flows.py#L68.
| model_config = self.config.get("model", {}) | ||
| provider_config = model_config.get("provider", {}) | ||
| self.model_id = model_config.get("model_id") | ||
| self.provider = provider_config.get("name") |
There was a problem hiding this comment.
Should we add validation here with descriptive error messages?
| config_path if config_path else str(self.DEFAULT_CONFIG_PATH) | ||
| ) | ||
|
|
||
| super().__init__( |
There was a problem hiding this comment.
Should we support extract_params in BasicLangExtractRecognizer?
Those parameters are needed for different scenarios,
for example for ollama if we use small llm we would need the max_char_buffer..
OllamaLangExtractRecognizer passes parameters like max_char_buffer, timeout, and num_ctx , max_workers, language_model_params, extraction_passes.
to the parent class, but BasicLangExtractRecognizer doesn't support these yet.
i have thought about something like this:
Extract optional parameters from config
extract_params = {}
if "max_char_buffer" in model_config:
extract_params["extract"] = {"max_char_buffer": model_config["max_char_buffer"]}
lang_model_params = {}
for key in ["timeout", "num_ctx"]:
if key in model_config:
lang_model_params[key] = model_config[key]
if lang_model_params:
extract_params["language_model"] = lang_model_params
super().__init__(
config_path=actual_config_path,
name="Basic LangExtract PII",
supported_language=supported_language,
extract_params=extract_params or None
)| ) | ||
|
|
||
| def _get_provider_params(self): | ||
| """Return Azure OpenAI-specific params.""" |
There was a problem hiding this comment.
please fix the doc string
| provider_kwargs=self.provider_kwargs, | ||
| ) | ||
|
|
||
| def _get_provider_params(self): |
There was a problem hiding this comment.
this method can be removed also from the parent and from the AzureOpenAILangExtractRecognizer recognizer... but keep the abstraction i guess
|
Good comments! I'm happy to make the requested changes, but it may need to be late this week or early next. |
No worries, whenever you get a chance :) |
|
@telackey |
Hi :) @telackey |
|
Yes, the holidays just intervened. |
3 participants
Change Description
Add a new, basic LangExtract-based recognizer class that is generic. The current implementations focus on ollama or azure support. This one instantiates an lx.ModelConfig from the yaml, so that it can specify different providers and custom configurations (eg, developed using Ollama via a LiteLLM OpenAI proxy).
Issue reference
Fixes #XX
Checklist