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

rephrase confusing message on monitor creation #67

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 15 additions & 29 deletions tests/helpers/test_monitor_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -128,16 +128,6 @@ def test_get_monitor_config(self) -> None:
assert isinstance(monitor_config, Dict)
for key in monitor_config.keys():
assert key in ['orgId', 'datasetId', 'granularity', 'metadata', 'allowPartialTargetBatches', 'analyzers', 'monitors']

def test_get_monitor_config_not_existing_dataset_id(self, caplog) -> None:
with caplog.at_level("WARNING"):
monitor_config = get_monitor_config(
org_id=ORG_ID,
dataset_id = "fake-dataset-id",
)

assert monitor_config is None
assert "Could not find a monitor config for fake-dataset-id" in caplog.text

def test_get_monitor(self) -> None:
monitor = get_monitor(
Expand All @@ -153,25 +143,21 @@ def test_get_monitor(self) -> None:
assert key in ['id', 'analyzerIds', 'schedule', 'mode', 'disabled', 'actions', 'metadata']


def test_get_monitor_with_wrong_configs(self, caplog) -> None:
with caplog.at_level("WARNING"):
monitor = get_monitor(
monitor_id="fake-monitor",
dataset_id=DATASET_ID,
org_id=ORG_ID
)
assert monitor is None
assert f"Could not find a monitor with id fake-monitor for {DATASET_ID}." in caplog.text
with caplog.at_level("WARNING"):
monitor = get_monitor(
monitor_id=MONITOR_ID,
dataset_id="fake-dataset-id",
org_id=ORG_ID
)

assert monitor is None
assert f"Could not find a monitor with id {MONITOR_ID} for fake-dataset-id." in caplog.text

def test_get_monitor_with_wrong_configs(self) -> None:
monitor = get_monitor(
monitor_id="fake-monitor",
dataset_id=DATASET_ID,
org_id=ORG_ID
)
assert monitor is None

monitor = get_monitor(
monitor_id=MONITOR_ID,
dataset_id="fake-dataset-id",
org_id=ORG_ID
)

assert monitor is None

def test_get_granularity(self) -> None:
granularity = get_model_granularity(org_id=ORG_ID, dataset_id=DATASET_ID)
Expand Down
20 changes: 13 additions & 7 deletions whylabs_toolkit/helpers/monitor_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,20 @@
logger = logging.getLogger(__name__)


# TODO create deactivate_monitor


def get_monitor_config(org_id: str, dataset_id: str, config: Config = Config()) -> Any:
api = get_monitor_api(config=config)
try:
monitor_config = api.get_monitor_config_v3(org_id=org_id, dataset_id=dataset_id)
logger.info(f"Found monitor config for {dataset_id}")
return monitor_config
except NotFoundException:
logger.warning(f"Could not find a monitor config for {dataset_id}")
logger.info(f"Could not find a monitor config for {dataset_id}")
return None
except ForbiddenException as e:
logger.warning(
f"You don't have access to monitor config for {dataset_id}. Did you set a correct WHYLABS_API_KEY?"
)
raise e


def get_monitor(
Expand All @@ -37,11 +40,14 @@ def get_monitor(
try:
monitor = api.get_monitor(org_id=org_id, dataset_id=dataset_id, monitor_id=monitor_id)
return monitor
except (ForbiddenException, NotFoundException):
except (NotFoundException):
logger.info(f"Didn't find a monitor with id {monitor_id} for {dataset_id}. Creating a new one...")
return None
except ForbiddenException as e:
logger.warning(
f"Could not find a monitor with id {monitor_id} for {dataset_id}." "Did you set a correct WHYLABS_API_KEY?"
f"You don't have access to monitor {monitor_id} for {dataset_id}. Did you set a correct WHYLABS_API_KEY?"
)
return None
raise e


def get_analyzer_ids(
Expand Down
27 changes: 12 additions & 15 deletions whylabs_toolkit/monitor/manager/monitor_setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,12 @@
from whylabs_toolkit.monitor.models import *
from whylabs_toolkit.monitor.models.analyzer.targets import ColumnGroups
from whylabs_toolkit.monitor.manager.credentials import MonitorCredentials
from whylabs_toolkit.helpers.monitor_helpers import get_analyzers, get_monitor, get_model_granularity
from whylabs_toolkit.helpers.monitor_helpers import (
get_analyzers,
get_monitor,
get_model_granularity,
get_monitor_config,
)
from whylabs_toolkit.helpers.config import Config


Expand Down Expand Up @@ -54,20 +59,16 @@ def __init__(self, monitor_id: str, dataset_id: Optional[str] = None, config: Co

self._prefill_properties()

def _check_if_monitor_exists(self) -> Any:
def _check_if_monitor_exists(self) -> Optional[Monitor]:
existing_monitor = get_monitor(
org_id=self.credentials.org_id,
dataset_id=self.credentials.dataset_id,
monitor_id=self.credentials.monitor_id,
config=self._config,
)
if existing_monitor:
existing_monitor = Monitor.parse_obj(existing_monitor)
logger.info(f"Got existing {self.credentials.monitor_id} from WhyLabs!")
else:
logger.info(f"Did not find a monitor with {self.credentials.monitor_id}, creating a new one.")
existing_monitor = None
return existing_monitor
if existing_monitor is not None:
return Monitor.parse_obj(existing_monitor)
return None

def _check_if_analyzer_exists(self) -> Any:
existing_analyzers = get_analyzers(
Expand All @@ -76,12 +77,8 @@ def _check_if_analyzer_exists(self) -> Any:
monitor_id=self.credentials.monitor_id,
config=self._config,
)
if existing_analyzers:
existing_analyzer = Analyzer.parse_obj(existing_analyzers[0]) # enforcing 1:1 relationship

else:
existing_analyzer = None
return existing_analyzer
if existing_analyzers is not None:
return Analyzer.parse_obj(existing_analyzers[0]) # enforcing 1:1 relationship

def _prefill_properties(self) -> None:
if self.monitor:
Expand Down