feat(classifier): Add support for excluding list of exact column names #9472
Conversation
github-actions
bot
added
the
ingestion
| @@ -66,6 +83,9 @@ class Config: | |||
| description="Factors and their weights to consider when predicting info types", | |||
| alias="prediction_factors_and_weights", | |||
| ) | |||
| ExcludeName: Optional[ExcludeNameFactorConfig] = Field( | |||
There was a problem hiding this comment.
| ExcludeName: Optional[ExcludeNameFactorConfig] = Field( | |
| ExcludeName: Optional[List[str]] = Field(default=None, description="List of exact column names to exclude from classification for this info type") |
There was a problem hiding this comment.
We do not need separate class as it can be represented as list of str directly..
| }, | ||
| } | ||
| ).config | ||
| if config.info_types_config["Email_Address"].ExcludeName is not None: |
There was a problem hiding this comment.
can remove this if/else by asserting presence of ExcludeName directly.
| if config.info_types_config["Email_Address"].ExcludeName is not None: | |
| assert config.info_types_config["Email_Address"].ExcludeName is not None |
| }, | ||
| } | ||
| ).config | ||
| assert config.info_types_config["Email_Address"].ExcludeName is None |
There was a problem hiding this comment.
Thank you for adding tests.
ethan-cartwright
force-pushed
the
classifier_exclusion_names_support
branch
from
2716727 to
d7d0430
Compare
| @@ -73,6 +73,14 @@ class Config: | |||
| description="Factors and their weights to consider when predicting info types", | |||
| alias="prediction_factors_and_weights", | |||
| ) | |||
| StripExclusionFormatting: bool = Field( | |||
| default=True, alias="strip_exclusion_formatting" | |||
| ) | |||
There was a problem hiding this comment.
StripExclusionFormatting is not an infotype level config. As per equivalent datahub-classify PR, its at global level and is set only once across all infotypes. Did you intend this global behavior ? In that case -> strip_exclusion_formatting should be in DataHubClassifierConfig class in same file . Also, need not use StripExclusionFormatting with alias strip_exclusion_formatting here . You can directly name the field as strip_exclusion_formatting .
| StripExclusionFormatting: bool = Field( | ||
| default=True, alias="strip_exclusion_formatting" | ||
| ) | ||
| ExcludeName: Optional[List[str]] = Field( |
There was a problem hiding this comment.
@ethan-cartwright could you also please update classification.md with these changes ? Unfortunately the configs are not automatically updated yet.
There was a problem hiding this comment.
LGTM, except one small comment regarding log levels on datahub-classify PR
done in this PR: acryldata/datahub-classify#21 |
Description
Extends the classification module's config to allow specification of an
ExcludeNamelist that is used in this datahub-classify PR to exclude a list of exact column namesChecklist