-
Notifications
You must be signed in to change notification settings - Fork 37
[FSSDK-11571] Python: Add holdout support and refactor decision logic in DefaultDecisionService #467
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
base: master
Are you sure you want to change the base?
[FSSDK-11571] Python: Add holdout support and refactor decision logic in DefaultDecisionService #467
Conversation
… in DefaultDecisionService
…o esra/FSSDK-11571_add_holdout_decision_service
| # Create Decision with holdout - experiment is None, holdout field contains the holdout dict | ||
| holdout_decision: Decision = Decision( | ||
| experiment=None, | ||
| variation=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think variation here needs to have variation assigned.
holdout_decision: Decision = Decision(
experiment=None, # check if this should be None or holdout
variation=variation, # Should contain the bucketed variation !!
source=enums.DecisionSources.HOLDOUT,
cmab_uuid=None,
holdout=holdout
)
Javascript has this:
if (bucketResult.result) {
const variation = configObj.variationIdMap[bucketResult.result];
if (variation) {
return {
result: {
experiment: holdout, // Holdout as experiment
variation: variation, // Variation set
decisionSource: DECISION_SOURCES.HOLDOUT
},
reasons: decideReasons
};
}
}
| variation: Optional[entities.Variation] | ||
| source: Optional[str] | ||
| cmab_uuid: Optional[str] | ||
| holdout: Optional[Dict[str, str]] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI says this:
Location: optimizely/decision_service.py:79-86
Issue: The Python implementation uses a different structure than JavaScript/Swift:
- Python: Stores holdout in separate holdout field, experiment=None, variation=None
- JavaScript: Stores holdout AS the experiment field, with variation properly set
- Swift: Returns variation directly, holdout passed as experiment in FeatureDecision
Current Python:
class Decision(NamedTuple):
experiment: Optional[entities.Experiment]
variation: Optional[entities.Variation]
source: Optional[str]
cmab_uuid: Optional[str]
holdout: Optional[Dict[str, str]] = None # ❌ Separate field
JavaScript (reference):
{
experiment: holdout, // ✅ Holdout stored as experiment
variation: variation, // ✅ Variation set
decisionSource: DECISION_SOURCES.HOLDOUT
}
Recommendation: Either:
- Store holdout in the experiment field (like JavaScript)
- Or ensure variation is properly set and document this design difference
| reasons.extend(holdout_decision['reasons']) | ||
|
|
||
| decision = holdout_decision['decision'] | ||
| if (decision.experiment is None and decision.variation is None and decision.holdout is None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, check decision logic here.
Suspicious Logic in get_decision_for_flag
if (decision.experiment is None and decision.variation is None and decision.holdout is
None):
continue
Problem: This checks if holdout is None, but since holdout is passed as a parameter to
get_variation_for_holdout, the holdout field in the returned Decision will never be
None when processing holdouts. This logic doesn't make sense.
Expected: Should check if user was actually bucketed:
if decision.variation is None: # User not bucketed into holdout
continue
✅ CORRECT IMPLEMENTATIONS (for reference)
JavaScript SDK:
if (bucketResult.result) {
const variation = configObj.variationIdMap[bucketResult.result];
if (variation) {
return {
result: {
experiment: holdout, // Holdout as experiment
variation: variation, // Variation set
decisionSource: DECISION_SOURCES.HOLDOUT
},
reasons: decideReasons
};
}
}
Swift SDK:
if let variation = bucketedVariation {
let featureDecision = FeatureDecision(
experiment: holdout, // Holdout as experiment
variation: variation, // Variation set
source: Constants.DecisionSource.holdout.rawValue
)
return DecisionResponse(result: featureDecision, reasons: reasons)
}
Summary
Test plan
PR checks
Issues