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.

Sign up for GitHub

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

Jump to bottom

Support Measured Rollouts and retire "owner" field #118

Merged
merged 6 commits into from
Jun 12, 2024
Merged

Support Measured Rollouts and retire "owner" field #118

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
789bac3
support MRs and retire owner
mrlevitas Jun 10, 2024
74a0e1e
bump reddit-decider
mrlevitas Jun 10, 2024
9bbcd34
drop owner
mrlevitas Jun 12, 2024
32352f7
lint
mrlevitas Jun 12, 2024
1d8e81a
reorder
mrlevitas Jun 12, 2024
1f4222e
lint 2
mrlevitas Jun 12, 2024
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
2 changes: 1 addition & 1 deletion docs/requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ docutils==0.16
Jinja2==2.11.2
MarkupSafe==1.1.1
pydocstyle==5.1.1
reddit-decider==1.7.0
reddit-decider==1.11.0
reddit-edgecontext==1.0.0a3
Sphinx==3.4.0
sphinx-autodoc-typehints==1.11.1
Expand Down
10 changes: 3 additions & 7 deletions reddit_decider/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,9 @@ class ExperimentConfig:
bucket_val: str
start_ts: int
stop_ts: int
owner: str
owner: None = None
emit_event: Optional[bool] = None
measured: Optional[bool] = None


class DeciderContext:
Expand Down Expand Up @@ -236,7 +237,6 @@ def _send_expose(self, event: str, exposure_fields: dict) -> None:
bucket_val,
start_ts,
stop_ts,
owner,
) = event.split("::::")
except ValueError:
logger.warning(
Expand All @@ -251,7 +251,6 @@ def _send_expose(self, event: str, exposure_fields: dict) -> None:
bucket_val=bucket_val,
start_ts=self._cast_to_int(start_ts),
stop_ts=self._cast_to_int(stop_ts),
owner=owner,
)

event_fields = {**event_fields, **{bucket_val: bucketing_value}}
Expand Down Expand Up @@ -279,7 +278,6 @@ def _send_expose_if_holdout(self, event: str, exposure_fields: dict) -> None:
bucket_val,
start_ts,
stop_ts,
owner,
) = event.split("::::")
except ValueError:
logger.warning(
Expand All @@ -299,7 +297,6 @@ def _send_expose_if_holdout(self, event: str, exposure_fields: dict) -> None:
bucket_val=bucket_val,
start_ts=self._cast_to_int(start_ts),
stop_ts=self._cast_to_int(stop_ts),
owner=owner,
)

event_fields = {**event_fields, **{bucket_val: bucketing_value}}
Expand Down Expand Up @@ -432,7 +429,6 @@ def expose(
bucket_val=feature.bucket_val,
start_ts=feature.start_ts,
stop_ts=feature.stop_ts,
owner=feature.owner,
)

self._event_logger.log(
Expand Down Expand Up @@ -920,8 +916,8 @@ def get_experiment(self, experiment_name: str) -> Optional[ExperimentConfig]:
bucket_val=feature.bucket_val,
start_ts=feature.start_ts,
stop_ts=feature.stop_ts,
owner=feature.owner,
emit_event=feature.emit_event,
measured=feature.measured,
)


Expand Down
2 changes: 1 addition & 1 deletion requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
-r requirements-transitive.txt
baseplate>=2.0.0a1,<3.0
black==21.4b2
reddit-decider==1.7.0
reddit-decider==1.11.0
flake8==3.9.1
mypy==0.790
prometheus-client>=0.12.0,<1.0
Expand Down
2 changes: 1 addition & 1 deletion setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
install_requires=[
"baseplate>=2.0.0a1,<3.0",
"reddit-edgecontext>=1.0.0a3,<2.0",
"reddit-decider~=1.7.0",
"reddit-decider~=1.11.0",
"typing_extensions>=3.10.0.0,<5.0",
],
package_data={"reddit_experiments": ["py.typed"], "reddit_decider": ["py.typed"]},
Expand Down
88 changes: 84 additions & 4 deletions tests/decider_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,7 @@ def assert_exposure_event_fields(
cfg = self.exp_base_config[experiment_name]
self.assertEqual(getattr(event_fields["experiment"], "id"), cfg["id"])
self.assertEqual(getattr(event_fields["experiment"], "name"), cfg["name"])
self.assertEqual(getattr(event_fields["experiment"], "owner"), cfg["owner"])
self.assertEqual(getattr(event_fields["experiment"], "owner"), None)
self.assertEqual(getattr(event_fields["experiment"], "version"), cfg["version"])
self.assertEqual(getattr(event_fields["experiment"], "bucket_val"), bucket_val)

Expand All @@ -520,7 +520,7 @@ def assert_minimal_exposure_event_fields(
cfg = self.exp_base_config[experiment_name]
self.assertEqual(getattr(event_fields["experiment"], "id"), cfg["id"])
self.assertEqual(getattr(event_fields["experiment"], "name"), cfg["name"])
self.assertEqual(getattr(event_fields["experiment"], "owner"), cfg["owner"])
self.assertEqual(getattr(event_fields["experiment"], "owner"), None)
self.assertEqual(getattr(event_fields["experiment"], "version"), cfg["version"])
self.assertEqual(getattr(event_fields["experiment"], "bucket_val"), bucket_val)

Expand Down Expand Up @@ -573,7 +573,7 @@ def test_none_returned_on_get_variant_call_with_bad_id(self):
self.assertEqual(self.event_logger.log.call_count, 0)

assert any(
"Partially loaded Decider: 1 features failed to load: {'test': 'Manifest parsing error: invalid type: string \"1\", expected u32'}"
'Partially loaded Decider: 1 feature(s) failed to load: {"test": ParsingError(Error("invalid type: string \\"1\\", expected u32", line: 0, column: 0))'
in x.getMessage()
for x in captured.records
)
Expand Down Expand Up @@ -1508,6 +1508,86 @@ def test_get_variant_with_disabled_exp(self):
# exposure assertions
self.assertEqual(self.event_logger.log.call_count, 0)

def test_range_variant_emit_event_override(self):
cfg = {
"feature_rollout_100": {
"id": 9110,
"name": "feature_rollout_100",
"enabled": True,
"version": "1",
"start_ts": 1522306800,
"stop_ts": 32533405261,
"owner": "[email protected]",
"type": "feature_rollout",
"emit_event": False,
"experiment": {
"variants": [
{
"name": "enabled",
"size": 1.0,
"range_end": 1.0,
"range_start": 0.0
}
],
"experiment_version": 1,
"shuffle_version": 0,
"bucket_val": "user_id",
"log_bucketing": False
}
},
"measured_rollout_100": {
"id": 9119,
"name": "measured_rollout_100",
"enabled": True,
"version": "1",
"start_ts": 1522306800,
"stop_ts": 32533405261,
"owner": "[email protected]",
"type": "range_variant",
"emit_event": False,
"measured": True,
"experiment": {
"variants": [
{
"name": "enabled",
"size": 1.0,
"range_end": 1.0,
"range_start": 0.0,
"emit_event_override": True
}
],
"experiment_version": 1,
"shuffle_version": 0,
"bucket_val": "user_id",
"log_bucketing": False
}
}
}

with create_temp_config_file(cfg) as f:
decider = setup_decider(f, self.dc, self.mock_span, self.event_logger)

self.assertEqual(self.event_logger.log.call_count, 0)
variant = decider.get_variant(experiment_name="feature_rollout_100")
self.assertEqual(variant, "enabled")

# FR does NOT emit exposure
self.assertEqual(self.event_logger.log.call_count, 0)

# FR is NOT "measured"
fr_cfg = decider.get_experiment(experiment_name="feature_rollout_100")
self.assertEqual(fr_cfg.measured, False)

variant = decider.get_variant(experiment_name="measured_rollout_100")
self.assertEqual(variant, "enabled")

# Measured Rollout DOES emit exposure (due to RV "emit_event_override" field)
self.assertEqual(self.event_logger.log.call_count, 1)

# MR IS "measured"
mr_cfg = decider.get_experiment(experiment_name="measured_rollout_100")
self.assertEqual(mr_cfg.measured, True)

def test_get_experiment(self):
with create_temp_config_file(self.exp_base_config) as f:
decider = setup_decider(f, self.dc, self.mock_span, self.event_logger)
Expand All @@ -1521,7 +1601,7 @@ def test_get_experiment(self):
self.assertEqual(experiment.bucket_val, cfg["experiment"]["bucket_val"])
self.assertEqual(experiment.start_ts, cfg["start_ts"])
self.assertEqual(experiment.stop_ts, cfg["stop_ts"])
self.assertEqual(experiment.owner, cfg["owner"])
self.assertEqual(experiment.owner, None)
self.assertEqual(experiment.emit_event, True)

def test_get_variant_without_expose_with_HG_as_control_1_and_child_returns_none_does_expose(
Expand Down
Loading