Skip to content
This repository was archived by the owner on Nov 8, 2024. It is now read-only.

Conversation

ploomiz
Copy link
Contributor

@ploomiz ploomiz commented Apr 30, 2022


labels: mergeable

Fixes: #issue

#2907

Motivation and Context

The randomization assignment function is used for assigning subjects to variations. It is described in detail in this design doc.

Description

This PR implements the same assignment logic as in the Node SDK.

How has this been tested?

It's important that the Python and Node SDK implementations produce the same assignment results. To make sure the implementations are consistent, we use shared test data stored in a google cloud storage bucket. Before the test starts, the test data is downloaded from GCP and written to a local directory.

The test data includes an expectedAssignments field which is the source of truth in that all SDK tests (iOS, Ruby, etc.) should verify these assignments are generated for the subjects input. How do we know the expectedAssignments are correct? Before uploading test data to the storage bucket, verify that expectedAssignments have an even variation split; each variation should have roughly the same number of assignments.

@ploomiz ploomiz self-assigned this Apr 30, 2022
@ploomiz ploomiz marked this pull request as ready for review April 30, 2022 23:48
@ploomiz ploomiz requested review from petzel, schmit and vpai May 2, 2022 14:48
Copy link
Contributor

@petzel petzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment on naming. Assignment logic and tests LGTM.

shard = get_shard(
"assignment-{}-{}".format(subject, flag), experiment_config.subjectShards
)
return next(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

from pydantic import BaseModel


def to_camel(s: str):
Copy link
Contributor Author

@ploomiz ploomiz May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this in response to #2 (comment)

@ploomiz
Copy link
Contributor Author

ploomiz commented May 3, 2022

@petzel @schmit I renamed flag -> experiment_key. Will do the same in the node SDK.

Otherwise does it look okay to you?

@ploomiz ploomiz merged commit d8fc8ad into main May 4, 2022
@ploomiz ploomiz deleted the assignment-function branch May 4, 2022 03:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants