From 40517b6a8d06d4e6e4c3768e6505518e98be78a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Murilo=20Menezes=20Mendon=C3=A7a?= Date: Wed, 29 May 2024 15:51:34 -0300 Subject: [PATCH] remove other notification actions (#64) * remove other notification actions details: - it has been quite a while since we only support GlobalActions - this PR removes both the helper methods to turn models into GlobalActions as well as typing references to other models - it might break existing customers using the tuned models, so the next release should communicate this properly - this PR also fixes a bunch of typing issues after running pyright on the repo - remove unused tests + use Annotated type --- pyproject.toml | 4 + tests/monitor/manager/test_manager.py | 65 +------------- whylabs_toolkit/helpers/monitor_helpers.py | 2 +- whylabs_toolkit/helpers/schema.py | 2 +- whylabs_toolkit/monitor/manager/manager.py | 62 +------------- .../monitor/manager/monitor_setup.py | 84 +++++++++---------- whylabs_toolkit/monitor/models/__init__.py | 5 -- .../monitor/models/analyzer/analyzer.py | 2 +- whylabs_toolkit/monitor/models/monitor.py | 64 ++++---------- 9 files changed, 69 insertions(+), 221 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index aa889ad..7562668 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -42,6 +42,10 @@ build-backend = "poetry.core.masonry.api" [tool.flake8] max-line-length = 140 +ignore = ["F405"] + +[tool.pyright] +include = ["whylabs_toolkit/**/*.py"] [tool.poetry.extras] diagnoser = ["pandas", "numpy", "tabulate", "isodate", "python-dateutil"] \ No newline at end of file diff --git a/tests/monitor/manager/test_manager.py b/tests/monitor/manager/test_manager.py index 76bf01a..a26418e 100644 --- a/tests/monitor/manager/test_manager.py +++ b/tests/monitor/manager/test_manager.py @@ -27,7 +27,7 @@ def test_validate(self, manager: MonitorManager) -> None: assert manager.validate() def test_failing_validation(self, monitor_setup: MonitorSetup) -> None: - monitor_setup.actions = [EmailRecipient(id="some_long_id", destination="someemail@email.com")] + monitor_setup.actions = [GlobalAction(target="some_long_id")] monitor_setup.config.mode = "weird_mode" # type: ignore monitor_setup.apply() @@ -77,66 +77,3 @@ def test_monitor_running_eagerly(self, existing_monitor_setup: MonitorSetup) -> ) assert new_expected_result["allowPartialTargetBatches"] == False - - -class TestNotificationActions(TestCase): - def setUp(self) -> None: - self.monitor_setup = MagicMock() - self.monitor_setup.credentials.org_id = 'test_org' - - self.monitor_setup.monitor = MagicMock() - self.monitor_setup.monitor.actions = [ - SlackWebhook(id='slack1', destination='https://slack.com/webhook'), - EmailRecipient(id='email1', destination='test@example.com'), - GlobalAction(target="existing-pagerDuty") - ] - - self.notifications_api = MagicMock() - self.notifications_api.list_notification_actions.return_value = [] - - self.monitor_api = MagicMock() - - self.monitor_manager = MonitorManager( - setup = self.monitor_setup, - notifications_api=self.notifications_api, - monitor_api=self.monitor_api - ) - - def test_notification_actions_are_updated(self) -> None: - self.monitor_manager._update_notification_actions() - - expected_calls = [ - call( - org_id='test_org', - type='EMAIL', - action_id='email1', - body={'email': 'test@example.com'} - ), - call( - org_id='test_org', - type='SLACK', - action_id='slack1', - body={'slackWebhook': 'https://slack.com/webhook'} - ) - ] - - for call_args in expected_calls: - assert call_args in self.notifications_api.put_notification_action.call_args_list - - def test_global_actions_are_made(self) -> None: - self.monitor_manager._update_notification_actions() - - assert GlobalAction(target='existing-pagerDuty') in self.monitor_setup.monitor.actions - - def test_existing_notification_actions_are_fetched(self) -> None: - self.monitor_manager._update_notification_actions() - - self.notifications_api.list_notification_actions.assert_called_once_with( - org_id='test_org' - ) - - def test_error_is_raised_if_monitor_is_none(self) -> None: - self.monitor_setup.monitor = None - - with self.assertRaises(ValueError): - self.monitor_manager._update_notification_actions() diff --git a/whylabs_toolkit/helpers/monitor_helpers.py b/whylabs_toolkit/helpers/monitor_helpers.py index fb32ee7..7fb0e59 100644 --- a/whylabs_toolkit/helpers/monitor_helpers.py +++ b/whylabs_toolkit/helpers/monitor_helpers.py @@ -122,5 +122,5 @@ def delete_monitor( resp_monitor = api.delete_monitor(org_id=org_id, dataset_id=dataset_id, monitor_id=monitor_id) logger.debug(f"Deleted monitor with Resp:{resp_monitor}") except ApiValueError as e: - logger.error(f"Error deleting monitor {monitor_id}: {e.msg}") + logger.error(f"Error deleting monitor {monitor_id}: {e.msg}") # type: ignore raise e diff --git a/whylabs_toolkit/helpers/schema.py b/whylabs_toolkit/helpers/schema.py index 19cc0fc..077315b 100644 --- a/whylabs_toolkit/helpers/schema.py +++ b/whylabs_toolkit/helpers/schema.py @@ -51,7 +51,7 @@ def _update_entity_schema(self) -> None: def _put_updated_entity_schema(self) -> None: metadata_dict = self.current_entity_schema["metadata"] entity_schema_dict = EntitySchema(columns=self.columns_dict, metadata=metadata_dict) - self._put_entity_schema(schema=entity_schema_dict) + self._put_entity_schema(schema=entity_schema_dict) # type: ignore def update(self) -> None: self._validate_input() diff --git a/whylabs_toolkit/monitor/manager/manager.py b/whylabs_toolkit/monitor/manager/manager.py index 5c1bf99..701107a 100644 --- a/whylabs_toolkit/monitor/manager/manager.py +++ b/whylabs_toolkit/monitor/manager/manager.py @@ -1,17 +1,16 @@ import logging import json from pathlib import Path -from typing import Optional, Union, Any, List +from typing import Optional, Any from jsonschema import validate, ValidationError -from whylabs_client.api.notification_settings_api import NotificationSettingsApi -from whylabs_client.api.models_api import ModelsApi +from whylabs_client.api.monitor_api import MonitorApi from whylabs_toolkit.monitor.manager.monitor_setup import MonitorSetup from whylabs_toolkit.monitor.models import * from whylabs_toolkit.helpers.monitor_helpers import get_model_granularity from whylabs_toolkit.helpers.config import Config -from whylabs_toolkit.helpers.utils import get_monitor_api, get_notification_api +from whylabs_toolkit.helpers.utils import get_monitor_api logging.basicConfig(level=logging.INFO) @@ -23,64 +22,13 @@ def __init__( self, setup: MonitorSetup, eager: Optional[bool] = None, - notifications_api: Optional[NotificationSettingsApi] = None, - monitor_api: Optional[ModelsApi] = None, + monitor_api: Optional[MonitorApi] = None, config: Config = Config(), ) -> None: self._setup = setup - self.__notifications_api = notifications_api or get_notification_api(config=config) self.__monitor_api = monitor_api or get_monitor_api(config=config) self.__eager = eager - def _get_existing_notification_actions(self) -> List[str]: - actions_dict_list = self.__notifications_api.list_notification_actions(org_id=self._setup.credentials.org_id) - action_ids = [] - for action in actions_dict_list: - action_ids.append(action.get("id")) - return action_ids - - @staticmethod - def get_notification_request_payload(action: Union[SlackWebhook, EmailRecipient, PagerDuty]) -> str: - if isinstance(action, SlackWebhook): - return "slackWebhook" - elif isinstance(action, EmailRecipient): - return "email" - elif isinstance(action, PagerDuty): - return "pagerDutyKey" - else: - raise ValueError( - f"Can't work with {action} type. Available options are SlackWebhook, PagerDuty and EmailRecipient." - ) - - def _update_notification_actions(self) -> None: - """ - Updates the notification actions to be passed to WhyLabs based on the actions defined in the MonitorBuilder object. - """ - if not self._setup.monitor: - raise ValueError("You must call apply() on your MonitorSetup object!") - - existing_actions = self._get_existing_notification_actions() - - for action in self._setup.monitor.actions: - if isinstance(action, GlobalAction): - continue - - if action.id not in existing_actions: - logger.info(f"Didn't find a {action.type} action under the ID {action.id}, creating one now!") - payload_key = self.get_notification_request_payload(action=action) - self.__notifications_api.put_notification_action( - org_id=self._setup.credentials.org_id, - type=action.type.upper(), - action_id=action.id, - body={payload_key: action.destination}, - ) - - if self._setup.monitor: - self._setup.monitor.actions = [ - action if isinstance(action, GlobalAction) else GlobalAction(target=action.id) - for action in self._setup.monitor.actions - ] - def _get_current_monitor_config(self) -> Optional[Any]: monitor_config = self.__monitor_api.get_monitor_config_v3( org_id=self._setup.credentials.org_id, dataset_id=self._setup.credentials.dataset_id @@ -88,8 +36,6 @@ def _get_current_monitor_config(self) -> Optional[Any]: return monitor_config def dump(self) -> Any: - self._update_notification_actions() - doc = Document( orgId=self._setup.credentials.org_id, datasetId=self._setup.credentials.dataset_id, diff --git a/whylabs_toolkit/monitor/manager/monitor_setup.py b/whylabs_toolkit/monitor/manager/monitor_setup.py index b22eca6..0c55e40 100644 --- a/whylabs_toolkit/monitor/manager/monitor_setup.py +++ b/whylabs_toolkit/monitor/manager/monitor_setup.py @@ -3,8 +3,6 @@ from datetime import datetime, timezone from typing import Optional, List, Union, Any -from whylabs_client.exceptions import NotFoundException - from whylabs_toolkit.helpers.utils import get_models_api from whylabs_toolkit.monitor.models import * from whylabs_toolkit.monitor.models.analyzer.targets import ColumnGroups @@ -30,25 +28,25 @@ def __init__(self, monitor_id: str, dataset_id: Optional[str] = None, config: Co self._models_api = get_models_api(config=self._config) self._monitor_mode: Optional[Union[EveryAnomalyMode, DigestMode]] = None - self._monitor_actions: Optional[List[Union[GlobalAction, EmailRecipient, SlackWebhook, PagerDuty]]] = None + self._monitor_actions: List[GlobalAction] = [] self._analyzer_schedule: Optional[Union[FixedCadenceSchedule, CronSchedule]] = None - self._target_matrix: Optional[Union[ColumnMatrix, DatasetMatrix]] = None - self._analyzer_config: Optional[ - Union[ - DiffConfig, - FixedThresholdsConfig, - StddevConfig, - DriftConfig, - ComparisonConfig, - SeasonalConfig, - FrequentStringComparisonConfig, - ListComparisonConfig, - ConjunctionConfig, - DisjunctionConfig, - ] - ] = None self._target_columns: Optional[List[str]] = [] self._exclude_columns: Optional[List[str]] = [] + self._target_matrix: Union[ColumnMatrix, DatasetMatrix] = ColumnMatrix( + include=self._target_columns, exclude=self._exclude_columns, segments=[] + ) + self._analyzer_config: Union[ + DiffConfig, + FixedThresholdsConfig, + StddevConfig, + DriftConfig, + ComparisonConfig, + SeasonalConfig, + FrequentStringComparisonConfig, + ListComparisonConfig, + ConjunctionConfig, + DisjunctionConfig, + ] self._monitor_tags: Optional[List[str]] = [] self._analyzer_tags: Optional[List[str]] = [] self._analyzer_disable_target_rollup: Optional[bool] = None @@ -116,19 +114,17 @@ def target_matrix(self, target: Union[ColumnMatrix, DatasetMatrix]) -> None: @property def config( self, - ) -> Optional[ - Union[ - DiffConfig, - FixedThresholdsConfig, - StddevConfig, - DriftConfig, - ComparisonConfig, - SeasonalConfig, - FrequentStringComparisonConfig, - ListComparisonConfig, - ConjunctionConfig, - DisjunctionConfig, - ] + ) -> Union[ + DiffConfig, + FixedThresholdsConfig, + StddevConfig, + DriftConfig, + ComparisonConfig, + SeasonalConfig, + FrequentStringComparisonConfig, + ListComparisonConfig, + ConjunctionConfig, + DisjunctionConfig, ]: return self._analyzer_config @@ -150,11 +146,11 @@ def config( self._analyzer_config = config @property - def actions(self) -> Optional[List[Union[GlobalAction, EmailRecipient, SlackWebhook, PagerDuty]]]: + def actions(self) -> Optional[List[GlobalAction]]: return self._monitor_actions @actions.setter - def actions(self, actions: List[Union[GlobalAction, EmailRecipient, SlackWebhook, PagerDuty]]) -> None: + def actions(self, actions: List[GlobalAction]) -> None: self._monitor_actions = actions @property @@ -238,9 +234,6 @@ def set_target_columns(self, columns: List[str]) -> None: """ if self._validate_columns_input(columns=columns): self._target_columns = columns - self._target_matrix = self._target_matrix or ColumnMatrix( - include=self._target_columns, exclude=self._exclude_columns, segments=[] - ) if isinstance(self._target_matrix, ColumnMatrix): self._target_matrix.include = self._target_columns @@ -271,6 +264,8 @@ def __set_analyzer(self) -> None: self.analyzer = Analyzer( id=self.credentials.analyzer_id, + metadata=None, + disabled=False, displayName=self.credentials.analyzer_id, disableTargetRollup=self._analyzer_disable_target_rollup, targetMatrix=self._target_matrix, @@ -281,10 +276,12 @@ def __set_analyzer(self) -> None: ) def __set_monitor( - self, monitor_mode: Optional[Union[EveryAnomalyMode, DigestMode]], monitor_actions: Optional[List[Any]] + self, monitor_mode: Union[EveryAnomalyMode, DigestMode], monitor_actions: List[GlobalAction] = [] ) -> None: self.monitor = Monitor( id=self.credentials.monitor_id, + metadata=None, + severity=None, disabled=False, displayName=self.credentials.monitor_id, tags=self._monitor_tags, @@ -309,7 +306,7 @@ def __set_dataset_matrix_for_dataset_metric(self) -> None: logger.warning( "ColumnMatrix is not configurable with a DatasetMetric." "Changing it to DatasetMatrix instead" ) - self._target_matrix = DatasetMatrix(segments=self._target_matrix.segments) + self._target_matrix = DatasetMatrix(type=TargetLevel.dataset, segments=self._target_matrix.segments) return None elif isinstance(self._target_matrix, DatasetMatrix) and not isinstance( @@ -337,7 +334,7 @@ def __set_dataset_matrix_for_missing_data_metric(self) -> None: "Missing data point needs to be set with target_matrix of type DatasetMatrix" "Changing to DatasetMatrix now." ) - self._target_matrix = DatasetMatrix(segments=self._target_matrix.segments) + self._target_matrix = DatasetMatrix(type=TargetLevel.dataset, segments=self._target_matrix.segments) return None if ( @@ -349,18 +346,19 @@ def __set_dataset_matrix_for_missing_data_metric(self) -> None: "secondsSinceLastUpload needs to be set with target_matrix of type DatasetMatrix" "Changing to DatasetMatrix now." ) - self._target_matrix = DatasetMatrix(segments=self._target_matrix.segments) + self._target_matrix = DatasetMatrix(type=TargetLevel.dataset, segments=self._target_matrix.segments) return None def apply(self) -> None: - monitor_mode = self._monitor_mode or DigestMode() - actions = self._monitor_actions or [] + monitor_mode = self._monitor_mode or DigestMode( + type="DIGEST", filter=None, creationTimeOffset=None, datasetTimestampOffset=None, groupBy=None + ) self._analyzer_schedule = self._analyzer_schedule or FixedCadenceSchedule( cadence=get_model_granularity( org_id=self.credentials.org_id, dataset_id=self.credentials.dataset_id # type: ignore ) ) - self.__set_monitor(monitor_mode=monitor_mode, monitor_actions=actions) + self.__set_monitor(monitor_mode=monitor_mode, monitor_actions=self._monitor_actions) self.__set_analyzer() diff --git a/whylabs_toolkit/monitor/models/__init__.py b/whylabs_toolkit/monitor/models/__init__.py index cf83c28..77210f0 100644 --- a/whylabs_toolkit/monitor/models/__init__.py +++ b/whylabs_toolkit/monitor/models/__init__.py @@ -57,11 +57,6 @@ "CronSchedule", "FixedCadenceSchedule", "Cadence", - # monitor actions - # "RawWebhook", - "SlackWebhook", - "EmailRecipient", - "PagerDuty", "GlobalAction", # big document "Document", diff --git a/whylabs_toolkit/monitor/models/analyzer/analyzer.py b/whylabs_toolkit/monitor/models/analyzer/analyzer.py index f9eac4c..33e5b3d 100644 --- a/whylabs_toolkit/monitor/models/analyzer/analyzer.py +++ b/whylabs_toolkit/monitor/models/analyzer/analyzer.py @@ -54,7 +54,7 @@ class Analyzer(NoExtrasBaseModel): regex="[0-9a-zA-Z \\-_]+", ) tags: Optional[ # type: ignore - List[constr(min_length=3, max_length=32, regex="[0-9a-zA-Z\\-_]")] # noqa + List[constr(min_length=3, max_length=32, regex="[0-9a-zA-Z\\-_]")] # type: ignore ] = Field( # noqa F722 None, description="A list of tags that are associated with the analyzer." ) diff --git a/whylabs_toolkit/monitor/models/monitor.py b/whylabs_toolkit/monitor/models/monitor.py index d11b4b1..e974945 100644 --- a/whylabs_toolkit/monitor/models/monitor.py +++ b/whylabs_toolkit/monitor/models/monitor.py @@ -1,8 +1,9 @@ """Schema for configuring a monitor.""" from enum import Enum from typing import Any, Dict, List, Literal, Optional, Union +from typing_extensions import Annotated -from pydantic import BaseModel, Field, constr, HttpUrl, parse_obj_as, validator +from pydantic import BaseModel, Field, constr from whylabs_toolkit.monitor.models.commons import ( @@ -30,43 +31,6 @@ class GlobalAction(NoExtrasBaseModel): target: str = Field(description="The unique action ID in the platform", regex="[a-zA-Z0-9\\-_]+", max_length=100) -class EmailRecipient(NoExtrasBaseModel): - """Action to send an email.""" - - type: Literal["email"] = "email" - id: str = Field(description="The e-mail ID to which you wish to send notifications to") - destination: str = Field(description="Destination email", format="email", max_length=1000) - - -class SlackWebhook(NoExtrasBaseModel): - """Action to send a Slack webhook.""" - - type: Literal["slack"] = "slack" - id: str = Field(description="The endpoint ID to which you wish to send notifications to") - destination: str = Field(description="The Slack target webhook endpoint") - - -class RawWebhook(NoExtrasBaseModel): - """Action to send a Raw webhook.""" - - type: Literal["raw"] = "raw" - id: str = Field(description="The endpoint ID to which you wish to send notifications to") - destination: Optional[str] = Field( - default=None, description="Sending raw unformatted message in JSON format to a webhook" - ) - - -class PagerDuty(NoExtrasBaseModel): - """Action to send a PagerDuty notification.""" - - type: Literal["pager_duty"] = "pager_duty" - id: str = Field(description="The PagerDuty endpoint ID to send notifications to") - destination: Optional[str] = Field( - default=None, - description="The secret key to access the PagerDuty endpoint. Required when the ID was not created", - ) - - class AnomalyFilter(NoExtrasBaseModel): """Filter the anomalies based on certain criteria. If the alerts are filtered down to 0, the monitor won't fire.""" @@ -216,15 +180,19 @@ class Monitor(NoExtrasBaseModel): max_length=256, regex="[0-9a-zA-Z \\-_]+", ) - tags: Optional[ # type: ignore - List[constr(min_length=3, max_length=32, regex="[0-9a-zA-Z\\-_]")] # noqa F722 - ] = Field(None, description="A list of tags that are associated with the monitor.") - analyzerIds: List[constr(regex="^[A-Za-z0-9_\\-]+$")] = Field( # type: ignore # noqa: F722 - title="AnalyzerIds", - description="The corresponding analyzer ID. Even though it's plural, we only support one analyzer at the " - "moment", - max_items=100, - ) + tags: Annotated[ + Optional[List[str]], + Field(title="Tags", description="The corresponding segment tags.", max_items=10, pattern="[0-9a-zA-Z\\-_]+$"), + ] = None + analyzerIds: Annotated[ + List[str], + Field( + title="AnalyzerIds", + description="The corresponding analyzer IDs for the conjunction.", + # max_items=10, + pattern="^[A-Za-z0-9_\\-]+$", + ), + ] schedule: Union[FixedCadenceSchedule, CronSchedule, ImmediateSchedule] = Field( description="Schedule of the monitor. We only support hourly monitor at " "the finest granularity", ) @@ -234,7 +202,7 @@ class Monitor(NoExtrasBaseModel): description="Notification mode and how we might handle different analysis", discriminator="type", ) - actions: List[Union[GlobalAction, EmailRecipient, SlackWebhook, PagerDuty]] = Field( + actions: List[GlobalAction] = Field( description="List of destination for the outgoing messages", max_items=100, )