PresidioSentenceFaker

[Robbie-Palmer]

1_Generate_data.ipynb and presidio_data_generator.py's main function contain code for generating fake records utilising the new providers and the mappings defined in this package.
This is a useful higher-level API for consuming the functionality of this package in other projects e.g.

from presidio_evaluator.data_generator import PresidioFakeRecordGenerator

entity_generator = PresidioFakeRecordGenerator(locale="en_US", lower_case_ratio=0.05,
                                               random_seed=42)
fake_patterns = entity_generator.generate_new_fake_records(num_samples=10000)

The version of the code in this PR takes from the notebook, which includes a mapping from Faker entity types to Presidio entity types, which isn't done in presidio_data_generator.py's main function.
Otherwise, it is the same code, just wrapped up into a class.

In this PR, I've also changed what I think are bugs.

• The presidio_entities_map which maps Faker entity types to Presidio entity types
  • Included "URL": "DOMAIN_NAME" which I believe should be the other way round
  • Included "DOMAIN_NAME": "DOMAIN_NAME" but DOMAIN_NAME isn't listed in https://microsoft.github.io/presidio/supported_entities/
• PresidioAnalyzerWrapper's predict method is hard coded to always use "en" instead of the provided language
• PresidioAnalyzerWrapper's construction logic around language is confusing, especially between providing an analyzer or not
  • Validated that if an analyzer is provided, the chosen language must be supported by the analyzer
  • If an analyzer is not provided, construct a new one, passing in the language as the only supported language

And made some stylistic changes which I think makes the code more readable, though obviously, this is subjective

• Dict literals to dict constructors
• Usage of / for creating pathlib Paths
• Usage of f strings
• Formatting from PyCharm, just because this is the IDE, I'm using, and it makes it easy to keep things tidy
  • If different formatting is preferred, it may be helpful to configure it using git pre-commit hooks, black and flake8, as outlined here: https://ljvmiranda921.github.io/notebook/2018/06/21/precommits-using-black-and-flake8/
    • I'm happy to do this if you think it is useful

[omri374]

Thanks @Robbie-Palmer. We'll review this soon.

[omri374]

Thanks @Robbie-Palmer, this is a great addition and greatly improves the codebase!
A few mimor comments:

1. Could you add a few unit-tests for the new class?
2. The distinction between the previous and suggest class mighit not be 100% clear to the user. Should we update the tutorials or add more clarification? Perhaps rename it to something else? (I don't have a suggestion, just a thought).

[omri374]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[omri374]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[omri374]

@Robbie-Palmer would you be interested in completing this PR? we'd be happy to help if needed

[Robbie-Palmer]

@omri374 I'm sorry for the really long delay! I've been pulled into a number of unrelated projects these past few months
No availability this coming week, but I'll prioritise addressing your comments asap afterwards

[omri374]

Thanks @Robbie-Palmer! Happy to collaborate and help on this if needed, there are really good contributions in this PR.

[omri374]

First of all, thank you @Robbie-Palmer! this PR introduces really great code improvements and bug fixes.

On the new class topic, I agree we should first align on the right structure. Backward compatibility is not a big issue IMHO as this package was just recently released to PyPI and we can create a deprecation notice for the old class name.

Here are the options as I see it:

1. Continue having one class
2. Have two classes, which is proposed in this PR
3. Have a base class, which is inherited by another class, similar to this PR but maintaining some relation between the two.

Another discussion was on the name(s) of classes.

It makes sense IMHO to have two classes, one serving as a wrapper for Faker and the other as an orchestrator. We should just think of the names in a way that wouldn't confuse, especially in light of the terms used in faker, e.g. Faker and Generator.

Happy to hear your thoughts @Robbie-Palmer and @melmatlis.
We could also set up a short call if that would be the easiest.

[omri374]

/azp run

[azure-pipelines]

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

[omri374]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).