fix(): fixed bugs with cache invalidation, notifications and guardrails
This commit is contained in:
@@ -5,6 +5,8 @@ from django.utils.translation import gettext_lazy as _
|
||||
|
||||
from apps.core.models import BaseModel
|
||||
|
||||
METRIC_DECIMAL_PLACES = 4
|
||||
|
||||
|
||||
class GuardrailAction(models.TextChoices):
|
||||
PAUSE = "pause", _("Pause experiment")
|
||||
@@ -26,7 +28,7 @@ class Guardrail(BaseModel):
|
||||
)
|
||||
threshold = models.DecimalField(
|
||||
max_digits=10,
|
||||
decimal_places=4,
|
||||
decimal_places=METRIC_DECIMAL_PLACES,
|
||||
verbose_name=_("threshold"),
|
||||
)
|
||||
observation_window_minutes = models.PositiveIntegerField(
|
||||
@@ -89,12 +91,12 @@ class GuardrailTrigger(BaseModel):
|
||||
)
|
||||
threshold = models.DecimalField(
|
||||
max_digits=10,
|
||||
decimal_places=4,
|
||||
decimal_places=METRIC_DECIMAL_PLACES,
|
||||
verbose_name=_("threshold"),
|
||||
)
|
||||
actual_value = models.DecimalField(
|
||||
max_digits=10,
|
||||
decimal_places=4,
|
||||
decimal_places=METRIC_DECIMAL_PLACES,
|
||||
verbose_name=_("actual value"),
|
||||
)
|
||||
observation_window_minutes = models.PositiveIntegerField(
|
||||
|
||||
@@ -2,13 +2,13 @@ from datetime import timedelta
|
||||
from decimal import Decimal
|
||||
from typing import Any
|
||||
|
||||
from django.core.cache import cache
|
||||
from django.core.exceptions import ValidationError
|
||||
from django.db import transaction
|
||||
from django.db.models import Case, QuerySet, Value, When
|
||||
from django.utils import timezone
|
||||
|
||||
from apps.experiments.models import (
|
||||
STARTED_STATUSES,
|
||||
Experiment,
|
||||
ExperimentLog,
|
||||
ExperimentOutcome,
|
||||
@@ -17,6 +17,7 @@ from apps.experiments.models import (
|
||||
OutcomeType,
|
||||
)
|
||||
from apps.guardrails.models import (
|
||||
METRIC_DECIMAL_PLACES,
|
||||
Guardrail,
|
||||
GuardrailAction,
|
||||
GuardrailTrigger,
|
||||
@@ -38,16 +39,6 @@ def guardrail_create(
|
||||
observation_window_minutes: int = 60,
|
||||
action: str = GuardrailAction.PAUSE,
|
||||
) -> Guardrail:
|
||||
if experiment.status in STARTED_STATUSES:
|
||||
raise ValidationError(
|
||||
{
|
||||
"experiment": (
|
||||
"Guardrails cannot be added after the experiment "
|
||||
"has been started "
|
||||
f"(status: '{experiment.status}')."
|
||||
)
|
||||
}
|
||||
)
|
||||
guardrail = Guardrail(
|
||||
experiment=experiment,
|
||||
metric=metric,
|
||||
@@ -65,16 +56,6 @@ def guardrail_update(
|
||||
**fields: Any,
|
||||
) -> Guardrail:
|
||||
guardrail.experiment.refresh_from_db(fields=["status"])
|
||||
if guardrail.experiment.status in STARTED_STATUSES:
|
||||
raise ValidationError(
|
||||
{
|
||||
"experiment": (
|
||||
"Guardrails cannot be modified after the experiment "
|
||||
"has been started "
|
||||
f"(status: '{guardrail.experiment.status}')."
|
||||
)
|
||||
}
|
||||
)
|
||||
allowed = {
|
||||
"threshold",
|
||||
"observation_window_minutes",
|
||||
@@ -93,16 +74,6 @@ def guardrail_update(
|
||||
|
||||
def guardrail_delete(*, guardrail: Guardrail) -> None:
|
||||
guardrail.experiment.refresh_from_db(fields=["status"])
|
||||
if guardrail.experiment.status in STARTED_STATUSES:
|
||||
raise ValidationError(
|
||||
{
|
||||
"experiment": (
|
||||
"Guardrails cannot be deleted after the experiment "
|
||||
"has been started "
|
||||
f"(status: '{guardrail.experiment.status}')."
|
||||
)
|
||||
}
|
||||
)
|
||||
guardrail.delete()
|
||||
|
||||
|
||||
@@ -138,8 +109,8 @@ def _calculate_guardrail_metric(
|
||||
metric=guardrail.metric,
|
||||
experiment_id=experiment.pk,
|
||||
variant_id=variant.pk,
|
||||
start_date=window_start,
|
||||
end_date=now,
|
||||
event_start_date=window_start,
|
||||
event_end_date=now,
|
||||
)
|
||||
if val is not None:
|
||||
values.append(val)
|
||||
@@ -182,7 +153,7 @@ def _execute_guardrail_action(
|
||||
experiment=experiment,
|
||||
metric_key=guardrail.metric.key,
|
||||
threshold=guardrail.threshold,
|
||||
actual_value=actual_value,
|
||||
actual_value=round(actual_value, METRIC_DECIMAL_PLACES),
|
||||
observation_window_minutes=guardrail.observation_window_minutes,
|
||||
action=guardrail.action,
|
||||
triggered_at=now,
|
||||
@@ -191,6 +162,7 @@ def _execute_guardrail_action(
|
||||
if guardrail.action == GuardrailAction.PAUSE:
|
||||
experiment.status = ExperimentStatus.PAUSED
|
||||
experiment.save(update_fields=["status", "updated_at"])
|
||||
cache.delete(f"active_exp:{experiment.flag_id}")
|
||||
|
||||
ExperimentLog.objects.create(
|
||||
experiment=experiment,
|
||||
@@ -218,6 +190,7 @@ def _execute_guardrail_action(
|
||||
|
||||
experiment.status = ExperimentStatus.COMPLETED
|
||||
experiment.save(update_fields=["status", "updated_at"])
|
||||
cache.delete(f"active_exp:{experiment.flag_id}")
|
||||
|
||||
ExperimentOutcome.objects.create(
|
||||
experiment=experiment,
|
||||
|
||||
@@ -1,6 +1,5 @@
|
||||
from decimal import Decimal
|
||||
|
||||
from django.core.exceptions import ValidationError
|
||||
from django.test import TestCase
|
||||
from django.utils import timezone
|
||||
|
||||
@@ -488,6 +487,119 @@ class CheckAllRunningTest(TestCase):
|
||||
self.assertGreater(len(results["triggers"]), 0)
|
||||
|
||||
|
||||
class GuardrailHigherIsBetterTest(TestCase):
|
||||
def setUp(self) -> None:
|
||||
review_settings_update(
|
||||
default_min_approvals=1,
|
||||
allow_any_approver=True,
|
||||
)
|
||||
self.approver = make_approver("_hib")
|
||||
|
||||
self.exposure_type = make_exposure_type()
|
||||
self.purchase_type = make_event_type(
|
||||
name="purchase",
|
||||
display_name="Purchase",
|
||||
requires_exposure=False,
|
||||
)
|
||||
|
||||
self.experiment = make_experiment(suffix="_hib")
|
||||
self.v_control, self.v_treatment = add_two_variants(self.experiment)
|
||||
|
||||
self.conversion_metric = metric_definition_create(
|
||||
key="hib_conversion_rate",
|
||||
name="Conversion Rate",
|
||||
metric_type=MetricType.RATIO,
|
||||
direction=MetricDirection.HIGHER_IS_BETTER,
|
||||
calculation_rule={
|
||||
"type": "ratio",
|
||||
"numerator_event": "purchase",
|
||||
"denominator_event": "exposure",
|
||||
},
|
||||
)
|
||||
|
||||
guardrail_create(
|
||||
experiment=self.experiment,
|
||||
metric=self.conversion_metric,
|
||||
threshold=Decimal("0.50"),
|
||||
observation_window_minutes=60,
|
||||
action=GuardrailAction.PAUSE,
|
||||
)
|
||||
|
||||
self.experiment = _start_experiment(self.experiment, self.approver)
|
||||
self.now = timezone.now()
|
||||
|
||||
def _create_decision_and_exposure(self, decision_id, subject_id, variant):
|
||||
decision_create(
|
||||
decision_id=decision_id,
|
||||
flag_key="flag_hib",
|
||||
subject_id=subject_id,
|
||||
experiment_id=str(self.experiment.pk),
|
||||
variant_id=str(variant.pk),
|
||||
value=variant.value,
|
||||
reason="experiment",
|
||||
)
|
||||
process_events_batch(
|
||||
[
|
||||
{
|
||||
"event_id": f"exp_{decision_id}",
|
||||
"event_type": "exposure",
|
||||
"decision_id": decision_id,
|
||||
"subject_id": subject_id,
|
||||
"timestamp": self.now.isoformat(),
|
||||
"properties": {},
|
||||
}
|
||||
]
|
||||
)
|
||||
|
||||
def _send_purchase(self, event_id, decision_id, subject_id):
|
||||
process_events_batch(
|
||||
[
|
||||
{
|
||||
"event_id": event_id,
|
||||
"event_type": "purchase",
|
||||
"decision_id": decision_id,
|
||||
"subject_id": subject_id,
|
||||
"timestamp": self.now.isoformat(),
|
||||
"properties": {},
|
||||
}
|
||||
]
|
||||
)
|
||||
|
||||
def test_trigger_when_conversion_below_threshold(self) -> None:
|
||||
for i in range(3):
|
||||
self._create_decision_and_exposure(
|
||||
f"dec_hib_{i}",
|
||||
f"u{i}",
|
||||
self.v_treatment,
|
||||
)
|
||||
self._send_purchase("pur_hib_0", "dec_hib_0", "u0")
|
||||
|
||||
triggers = check_experiment_guardrails(self.experiment)
|
||||
|
||||
self.assertEqual(len(triggers), 1)
|
||||
self.experiment.refresh_from_db()
|
||||
self.assertEqual(self.experiment.status, ExperimentStatus.PAUSED)
|
||||
self.assertLess(triggers[0].actual_value, Decimal("0.50"))
|
||||
|
||||
def test_no_trigger_when_conversion_above_threshold(self) -> None:
|
||||
for i in range(3):
|
||||
self._create_decision_and_exposure(
|
||||
f"dec_hib_ok_{i}",
|
||||
f"u{i}",
|
||||
self.v_treatment,
|
||||
)
|
||||
for i in range(3):
|
||||
self._send_purchase(
|
||||
f"pur_hib_ok_{i}", f"dec_hib_ok_{i}", f"u{i}"
|
||||
)
|
||||
|
||||
triggers = check_experiment_guardrails(self.experiment)
|
||||
|
||||
self.assertEqual(len(triggers), 0)
|
||||
self.experiment.refresh_from_db()
|
||||
self.assertEqual(self.experiment.status, ExperimentStatus.RUNNING)
|
||||
|
||||
|
||||
class GuardrailServiceTest(TestCase):
|
||||
def setUp(self) -> None:
|
||||
self.experiment = make_experiment(suffix="_gr")
|
||||
@@ -535,7 +647,7 @@ class GuardrailServiceTest(TestCase):
|
||||
)
|
||||
self.assertEqual(updated.threshold, Decimal("0.10"))
|
||||
|
||||
def test_reject_update_after_start(self) -> None:
|
||||
def test_update_guardrail_after_start(self) -> None:
|
||||
review_settings_update(
|
||||
default_min_approvals=1, allow_any_approver=True
|
||||
)
|
||||
@@ -552,8 +664,8 @@ class GuardrailServiceTest(TestCase):
|
||||
)
|
||||
exp = experiment_approve(experiment=exp, approver=approver)
|
||||
experiment_start(experiment=exp, user=self.experiment.owner)
|
||||
with self.assertRaises(ValidationError):
|
||||
guardrail_update(guardrail=g, threshold=Decimal("0.10"))
|
||||
updated = guardrail_update(guardrail=g, threshold=Decimal("0.10"))
|
||||
self.assertEqual(updated.threshold, Decimal("0.10"))
|
||||
|
||||
def test_delete_guardrail_in_draft(self) -> None:
|
||||
g = guardrail_create(
|
||||
@@ -564,7 +676,7 @@ class GuardrailServiceTest(TestCase):
|
||||
guardrail_delete(guardrail=g)
|
||||
self.assertEqual(Guardrail.objects.count(), 0)
|
||||
|
||||
def test_reject_delete_after_start(self) -> None:
|
||||
def test_delete_guardrail_after_start(self) -> None:
|
||||
review_settings_update(
|
||||
default_min_approvals=1, allow_any_approver=True
|
||||
)
|
||||
@@ -581,5 +693,5 @@ class GuardrailServiceTest(TestCase):
|
||||
)
|
||||
exp = experiment_approve(experiment=exp, approver=approver)
|
||||
experiment_start(experiment=exp, user=self.experiment.owner)
|
||||
with self.assertRaises(ValidationError):
|
||||
guardrail_delete(guardrail=g)
|
||||
guardrail_delete(guardrail=g)
|
||||
self.assertEqual(Guardrail.objects.count(), 0)
|
||||
|
||||
Reference in New Issue
Block a user