Skip to content

[AI-FSSDK] [FSSDK-12958] Continue rule evaluation after local holdout hit #524

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

Open
alexjoeyyong wants to merge 1 commit into
base: master
Choose a base branch
from
+194 −97
Open

[AI-FSSDK] [FSSDK-12958] Continue rule evaluation after local holdout hit #524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
94 changes: 49 additions & 45 deletions optimizely/decision_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class VariationResult(TypedDict):


class _DecisionResultOptional(TypedDict, total=False):
holdout_decision: Decision
holdout_decisions: List[Decision]


class DecisionResult(_DecisionResultOptional):
Expand Down Expand Up @@ -552,7 +552,8 @@ def get_variation(
}

def get_variation_for_rollout(
self, project_config: ProjectConfig, feature: entities.FeatureFlag, user_context: OptimizelyUserContext
self, project_config: ProjectConfig, feature: entities.FeatureFlag, user_context: OptimizelyUserContext,
holdout_decisions: Optional[list[Decision]] = None
) -> tuple[Decision, list[str]]:
""" Determine which experiment/variation the user is in for a given rollout.
Returns the variation of the first experiment the user qualifies for.
Expand All @@ -563,6 +564,7 @@ def get_variation_for_rollout(
rollout: Rollout for which we are getting the variation.
user: ID and attributes for user.
options: Decide options.
holdout_decisions: If given, local holdout hits are appended to it.

Returns:
Decision namedtuple consisting of experiment and variation for the user and
Expand Down Expand Up @@ -606,22 +608,14 @@ def get_variation_for_rollout(
return Decision(experiment=rule, variation=forced_decision_variation,
source=enums.DecisionSources.ROLLOUT, cmab_uuid=None), decide_reasons

local_holdouts = project_config.get_holdouts_for_rule(rule.id)
for holdout in local_holdouts:
local_holdout_decision = self.get_variation_for_holdout(
holdout, user_context, project_config
)
decide_reasons.extend(local_holdout_decision['reasons'])

local_decision = local_holdout_decision['decision']
if local_decision.variation is not None:
message = (
f"The user '{user_id}' is bucketed into local holdout '{holdout.key}' "
f"for delivery rule '{rule.key}'."
)
self.logger.info(message)
decide_reasons.append(message)
return local_decision, decide_reasons
local_holdout_hit = self._find_local_holdout_hit(
project_config, rule, 'delivery', user_context, decide_reasons
)
if local_holdout_hit is not None:
if holdout_decisions is not None:
holdout_decisions.append(local_holdout_hit)
index += 1
continue

bucketing_id, bucket_reasons = self._get_bucketing_id(user_id, attributes)
decide_reasons += bucket_reasons
Expand Down Expand Up @@ -748,6 +742,7 @@ def get_decision_for_flag(

global_holdout_result: DecisionResult | None = None
global_holdout_key: str | None = None
holdout_decisions: list[Decision] = []

# Check global holdouts (flag level — before any rules are evaluated)
global_holdouts = project_config.get_global_holdouts()
Expand Down Expand Up @@ -781,6 +776,7 @@ def get_decision_for_flag(
reasons.append(message)
global_holdout_result = holdout_decision
global_holdout_key = holdout.key
holdout_decisions.append(holdout_decision['decision'])
break

# Check experiments then rollouts
Expand All @@ -807,30 +803,16 @@ def get_decision_for_flag(
'error': False,
'reasons': reasons
}
if global_holdout_result is not None:
result['holdout_decision'] = global_holdout_result['decision']
if holdout_decisions:
result['holdout_decisions'] = holdout_decisions
return result

local_holdouts = project_config.get_holdouts_for_rule(experiment.id)
for holdout in local_holdouts:
local_holdout_decision = self.get_variation_for_holdout(
holdout, user_context, project_config
)
reasons.extend(local_holdout_decision['reasons'])

local_decision = local_holdout_decision['decision']
if local_decision.variation is not None:
message = (
f"The user '{user_id}' is bucketed into local holdout '{holdout.key}' "
f"for experiment rule '{experiment.key}'."
)
self.logger.info(message)
reasons.append(message)
return {
'decision': local_holdout_decision['decision'],
'error': False,
'reasons': reasons
}
local_holdout_hit = self._find_local_holdout_hit(
project_config, experiment, 'experiment', user_context, reasons
)
if local_holdout_hit is not None:
holdout_decisions.append(local_holdout_hit)
continue

# Get variation for experiment
variation_result = self.get_variation(
Expand All @@ -856,8 +838,8 @@ def get_decision_for_flag(
'error': False,
'reasons': reasons
}
if global_holdout_result is not None:
result['holdout_decision'] = global_holdout_result['decision']
if holdout_decisions:
result['holdout_decisions'] = holdout_decisions
return result

if global_holdout_result is not None:
Expand All @@ -870,7 +852,7 @@ def get_decision_for_flag(

# If no experiment decision, check rollouts
rollout_decision, rollout_reasons = self.get_variation_for_rollout(
project_config, feature_flag, user_context
project_config, feature_flag, user_context, holdout_decisions
)
if rollout_reasons:
reasons.extend(rollout_reasons)
Expand All @@ -893,10 +875,32 @@ def get_decision_for_flag(
'error': False,
'reasons': reasons
}
if global_holdout_result is not None:
final_result['holdout_decision'] = global_holdout_result['decision']
if holdout_decisions:
final_result['holdout_decisions'] = holdout_decisions
return final_result

def _find_local_holdout_hit(
self,
project_config: ProjectConfig,
rule: entities.Experiment,
rule_type: str,
user_context: OptimizelyUserContext,
reasons: list[str]
) -> Optional[Decision]:
"""Returns the decision of the first local holdout on the rule that buckets the user, else None."""
for holdout in project_config.get_holdouts_for_rule(rule.id):
holdout_result = self.get_variation_for_holdout(holdout, user_context, project_config)
reasons.extend(holdout_result['reasons'])
if holdout_result['decision'].variation is not None:
message = (
f"The user '{user_context.user_id}' is bucketed into local holdout '{holdout.key}' "
f"for {rule_type} rule '{rule.key}'. Skipping to the next rule."
)
self.logger.info(message)
reasons.append(message)
return holdout_result['decision']
return None

def get_variation_for_holdout(
self,
holdout: entities.Holdout,
Expand Down
42 changes: 20 additions & 22 deletions optimizely/optimizely.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1246,7 +1246,7 @@ def _create_optimizely_decision(
decision_reasons: Optional[list[str]],
decide_options: list[str],
project_config: ProjectConfig,
holdout_decision: Optional[Decision] = None
holdout_decisions: Optional[list[Decision]] = None
) -> OptimizelyDecision:
user_id = user_context.user_id
feature_enabled = False
Expand All @@ -1265,24 +1265,22 @@ def _create_optimizely_decision(

feature_flag = project_config.feature_key_map.get(flag_key)

# Send holdout impression when user was bucketed into a holdout bypassed due to exclude_targeted_deliveries
if (holdout_decision is not None
and decision_source != DecisionSources.HOLDOUT
and OptimizelyDecideOption.DISABLE_DECISION_EVENT not in decide_options):
holdout_enabled = self._get_feature_enabled(holdout_decision.variation)
holdout_rule_key = holdout_decision.experiment.key if holdout_decision.experiment else ''
self._send_impression_event(
project_config,
holdout_decision.experiment,
holdout_decision.variation,
flag_key,
holdout_rule_key,
str(DecisionSources.HOLDOUT),
holdout_enabled,
user_id,
attributes,
holdout_decision.cmab_uuid
)
# Holdouts the user was bucketed into without ending evaluation: local holdout hits and
# global holdouts bypassed due to exclude_targeted_deliveries. Sent regardless of send_flag_decisions.
if holdout_decisions and OptimizelyDecideOption.DISABLE_DECISION_EVENT not in decide_options:
for holdout_decision in holdout_decisions:
self._send_impression_event(
project_config,
holdout_decision.experiment,
holdout_decision.variation,
flag_key,
holdout_decision.experiment.key if holdout_decision.experiment else '',
str(DecisionSources.HOLDOUT),
self._get_feature_enabled(holdout_decision.variation),
user_id,
attributes,
holdout_decision.cmab_uuid
)
decision_event_dispatched = True

# Send impression event if Decision came from a feature
Expand Down Expand Up @@ -1469,13 +1467,13 @@ def _decide_for_keys(
user_context,
merged_decide_options
)
holdout_decisions: dict[str, Optional[Decision]] = {}
holdout_decisions: dict[str, list[Decision]] = {}
for i in range(0, len(flags_without_forced_decision)):
decision = decision_list[i]['decision']
reasons = decision_list[i]['reasons']
error = decision_list[i]['error']
flag_key = flags_without_forced_decision[i].key
holdout_decisions[flag_key] = decision_list[i].get('holdout_decision')
holdout_decisions[flag_key] = decision_list[i].get('holdout_decisions', [])
# store error decision against key and remove key from valid keys
if error:
optimizely_decision = OptimizelyDecision.new_error_decision(flags_without_forced_decision[i].key,
Expand All @@ -1496,7 +1494,7 @@ def _decide_for_keys(
decision_reasons,
merged_decide_options,
project_config,
holdout_decision=holdout_decisions.get(key)
holdout_decisions=holdout_decisions.get(key)
)
enabled_flags_only_missing = OptimizelyDecideOption.ENABLED_FLAGS_ONLY not in merged_decide_options
is_enabled = optimizely_decision.enabled
Expand Down
2 changes: 1 addition & 1 deletion tests/test_decision_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1510,7 +1510,7 @@ def test_get_variation_for_feature__returns_variation_for_feature_in_rollout(sel
)

mock_get_variation_for_rollout.assert_called_once_with(
self.project_config, feature, user
self.project_config, feature, user, []
)

# Assert no log messages were generated
Expand Down
Loading
Loading