Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

load analyzer configuration from file #1337

Merged

Conversation

roeybc
Copy link
Collaborator

@roeybc roeybc commented Mar 20, 2024

load analyzer configuration from file

giving the option to load analyzer from configuration file. It'll enable loading of:

  1. NLP Engine
  2. Reconizers
  3. Supported languages

Issue reference

This PR fixes issue #1338

Checklist

  • I have reviewed the contribution guidelines
  • I have signed the CLA (if required)
  • My code includes unit tests
  • All unit tests and lint checks pass locally
  • My PR contains documentation updates / additions if required

@roeybc roeybc changed the title initial version of loader load analyzer configuration from file Mar 20, 2024
Copy link
Contributor

@omri374 omri374 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This is a great addition which would allow easy configuration not just for the Docker based capabilities, but also for a better code-free configuration. Added a few comments, but happy to discuss each of those as most of them are mainly a matter of style rather than correctness.

presidio-analyzer/presidio_analyzer/analyzer_loader.py Outdated Show resolved Hide resolved
presidio-analyzer/presidio_analyzer/analyzer_loader.py Outdated Show resolved Hide resolved
presidio-analyzer/conf/analyzer.yaml Outdated Show resolved Hide resolved
presidio-analyzer/conf/analyzer.yaml Outdated Show resolved Hide resolved
presidio-analyzer/presidio_analyzer/analyzer_loader.py Outdated Show resolved Hide resolved
presidio-analyzer/conf/analyzer.yaml Outdated Show resolved Hide resolved
presidio-analyzer/presidio_analyzer/analyzer_loader.py Outdated Show resolved Hide resolved
@omri374
Copy link
Contributor

omri374 commented Mar 20, 2024

Fixes #1338

presidio-analyzer/conf/analyzer.yaml Outdated Show resolved Hide resolved
presidio-analyzer/conf/analyzer.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@omri374 omri374 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Left lots of comments but it's definitely in the right direction. Feel free to reach out if you want to discuss any of those.

presidio-analyzer/app.py Outdated Show resolved Hide resolved
presidio-analyzer/conf/analyzer.yaml Outdated Show resolved Hide resolved
presidio-analyzer/conf/analyzer.yaml Outdated Show resolved Hide resolved
presidio-analyzer/conf/analyzer.yaml Outdated Show resolved Hide resolved

return recognizers

def _load_recognizer_registry(self) -> RecognizerRegistry:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def _load_recognizer_registry(self) -> RecognizerRegistry:
def _load_recognizer_registry(self) -> Optional[RecognizerRegistry]:

Copy link
Contributor

@SharonHart SharonHart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks great, left some minor comments

@GautierT
Copy link

GautierT commented Apr 4, 2024

Hey.
When trying to call POST /analyze with this PR installed with docker-compose-transformers.yml and nothing else changed I have the error

{
    "error": "'list' object has no attribute 'supported_language'"
}

Logs :

2024-04-04 13:26:08,545 - presidio-analyzer - ERROR - A fatal error occurred during execution of AnalyzerEngine.analyze(). 'list' object has no attribute 'supported_language'

@omri374
Copy link
Contributor

omri374 commented Apr 11, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@omri374
Copy link
Contributor

omri374 commented Apr 13, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

SharonHart
SharonHart previously approved these changes Apr 15, 2024
@SharonHart
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@omri374
Copy link
Contributor

omri374 commented Apr 21, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@omri374 omri374 changed the base branch from main to feature/engines_from_conf April 21, 2024 14:50
@omri374 omri374 merged commit db37325 into microsoft:feature/engines_from_conf Apr 21, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants