feat(nimbus): Celery tasks to run holdback analysis weekly#16136
feat(nimbus): Celery tasks to run holdback analysis weekly#16136yashikakhurana wants to merge 12 commits into
Conversation
yashikakhurana
requested review from
freshstrangemusic,
jaredlockhart and
mikewilli
as code owners
yashikakhurana
changed the title
|
Blocked on this #16096 |
…riod and trigger Jetstream rerun
…x test expectations Holdback experiments never pause enrollment so actual_enrollment_end_date is always None, causing the task to skip every experiment. Switch to computed_enrollment_end_date (start_date + proposed_enrollment) which correctly reflects when the initial enrollment window expires. Update test comment and expected value to match the corrected calculation (36 days since end → weeks_elapsed=5 → 14+35=49).
…sized context managers
…back - Run daily instead of weekly (holdbacks can launch any day) - Use today - 21 days as rolling enrollment_end (observation period) - Skip experiments with end_date set (actually ended holdbacks) - Update _enrollment_end_date instead of proposed_enrollment - Add changelog entry per updated experiment - Compute proposedEnrollment, enrollmentEndDate, endDate on-the-fly in v8 serializer for active holdbacks — real fields stay untouched - Add HOLDBACK_ENROLLMENT_UPDATED changelog message constant
…on _enrollment_end_date
… serializer methods
mikewilli
left a comment
mikewilli
left a comment
There was a problem hiding this comment.
I suggested a couple small changes, but more importantly I think there's still some confusion over which dates to fake, when to fake them, and what they should be set to. My understanding of what we want to do here isn't quite in line with what you have here, and I'm not totally sure if that's a misunderstanding on my part or if I'm just not quite explaining it clearly enough.
Let me know if you want to chat about it to try and make sure we're on the same page.
| raise | ||
|
|
||
|
|
||
| HOLDBACK_OBSERVATION_DAYS = 21 |
There was a problem hiding this comment.
Constants shouldn't appear in the middle of the file.
| def get_enrollmentEndDate(self, obj): | ||
| enrollment_end = obj.actual_enrollment_end_date | ||
| if obj.is_holdback and not obj.end_date and enrollment_end: | ||
| return enrollment_end.isoformat() |
There was a problem hiding this comment.
I think this should be today - 21 days, right?
|
|
||
| def get_enrollmentEndDate(self, obj): | ||
| enrollment_end = obj.actual_enrollment_end_date | ||
| if obj.is_holdback and not obj.end_date and enrollment_end: |
There was a problem hiding this comment.
| if obj.is_holdback and not obj.end_date and enrollment_end: | |
| if obj.is_holdback and not obj.end_date and not enrollment_end: |
| def get_endDate(self, obj): | ||
| enrollment_end = obj.actual_enrollment_end_date | ||
| if obj.is_holdback and not obj.end_date and enrollment_end: | ||
| return (enrollment_end + datetime.timedelta(days=21)).isoformat() |
There was a problem hiding this comment.
This 21 should also be a constant, ideally the same one that we use in the jetstream task code. Maybe it can go in the django constants/settings?
| is_holdback=True, | ||
| status=NimbusExperiment.Status.LIVE, | ||
| _end_date=None, | ||
| ).exclude(_start_date=None) |
There was a problem hiding this comment.
Theoretically it shouldn't be possible for an experiment to be LIVE and have no start date, right? Not a big deal though if you think this could cover some edge case or something.
| NimbusExperiment.objects.filter(pk=experiment.pk).update( | ||
| _enrollment_end_date=enrollment_end, | ||
| do_rerun=True, | ||
| do_rerun_timestamp=now, | ||
| ) |
There was a problem hiding this comment.
Is changing the enrollment end date like this going to affect actual enrollments?
There was a problem hiding this comment.
I left some comments here and then went and looked at the task. I think what you have here makes sense IF it's valid to change the actual enrollment end date on the experiment, but I worry that this might break something else. I think it'd be better to assume the enrollment end date is NOT set until the experiment owner actually ends enrollment, and until then you can set a fake one in the serializer here the same way you do for the other dates.
If it is valid to change the actual enrollment end date, then maybe what you have here is ok.
2 participants
Because
This commit
Fixes #16086