Decider

[mrlevitas]

Summary

This pr creates these classes:

• DeciderContextFactory (for baseplate setup)
• DeciderContext (contains all fields used for bucketing, targeting, & exposure, e.g. "user_id", "country_code", etc)
• Decider (contains new API: get_variant())

as well as the baseplate setup convenience function:

• decider_client_from_config()

Todo:

• DeciderClient (convenience class for baseplate setup)
• implement get_variant_without_expose(), expose() on Decider

Expose logic has been implemented but commented out because Rust Decider lib needs to return a vector of events containing all of these fields:

“experiment_id:experiment_name:experiment_version:variant_name:bucket_val:start_ts:stop_ts:owner:event_type"

for the exposure event (currently, the SDK is not json.parse()ing the experiment cfg json and deferring that to Rust Decider lib, so Rust has to return these fields).

Testing

Unit tests for:

• get_variant() happy path
• get_variant() returns None if the experiment's name is not in the config
• get_variant() returns None if the json config has an experiment's id as a str instead of an int
• get_variant() returns None if missing "experiment" key in json config

Also some basic unit test coverage of:

• decider_client_from_config() setup
• that DeciderContextFactory.make_object_for_context() correctly populates DeciderContext ivar in Decider instance

Missing tests/todos:

• DeciderClient
• get_variant_without_expose()
• expose()

[mrlevitas]

CI will fail until we actually publish decider-py package to pypi, not just test.pypi.org.
@mattknox should we do this today so we can get tests running here?

edit: package published to pypi as reddit-decider and CI works.

[mrlevitas]

you can run pytest locally by installing decider-py package from test.pypi.org:
pip install -i https://test.pypi.org/simple/ decider-py==0.0.3

edit: pip install -r requirements.txt works

and then running the new test via:
python -m pytest tests/decider_tests.py
or make test for the full test suite.

[mattknox]

nice! You should probably have a BP.py expert like Andrew Boyle or similar look at this too.

[mattknox]

no event fields? That seems fine, if this is to be passed into decider

[mrlevitas]

user_event fields currently get passed under inputs keyword in event_logger.log() so I separated them out into get_user_event_fields() function here which I combine with additional exposure_kwargs passed into the fns ,in the commented out expose code, like so:

inputs = self._decider_context.get_user_event_fields()
inputs.update(exposure_kwargs or {})

[mrlevitas]

actually, I should combine self._decider_context.to_dict() into inputs as well to make some of the other fields available, such as bucketing_value, to the ExperimentsLogger, since it seems everything in input will get dumped to the final v2 event.

[mrlevitas]

removed get_user_event_fields() since self._decider_context.to_dict() contains the 3 fields that event_fields() contains

[julieahnt]

What will be the case to cause the id to be passed as str?

[mrlevitas]

just some general error handling, nothing particular--I took this test case from existing tests

[julieahnt]

I see.
It is a non blocker but I think if ZK config is somehow passing str id, maybe we should not silently return None.

[mrlevitas]

It's definitely a serious issue if that happens but I don't think we should raise an exception..at least it seems that the mentality of the experiments.py sdk is to log to warning instead of raising exceptions, which I do here (rust lib provides specific failure message to std out as well):

err: SerdeError(
    Error("invalid type: string \"1\", expected u32", line: 1, column: 19),
)
WARNING  reddit_decider:__init__.py:139 Rust decider did not initialize.

[mrlevitas]

maybe Rust decider did not initialize. should be error level log, instead of warning though?

[mrlevitas]

I made the x = rust_decider.init() return value have a way to access the error string via x.err() (in this pr) so that we can control the error message in the sdk and log it to logger.error() without having rust lib print to std out and pollute logs.

[mrlevitas]

getting user_id & logged_in from ec.user.event_fields() instead off of edge_context.user directly to match current functionality.

[mrlevitas]

nice! You should probably have a BP.py expert like Andrew Boyle or similar look at this too.

Hi @pacejackson!
@mattknox suggested I get run this change by your since it's intended to be pulled into BP.py
More details from the design doc here.

[SiddharthManoj]

Looks great Roman! I noticed some functions don't have return types - do we wanna just always add return types moving forward? (e.g. to_dict())