Skip to content

Support custom Decider context and identifier fields - #128

Draft
tim-zhu-reddit wants to merge 5 commits into
developfrom
codex/experiments-custom-field-fallback
Draft

tim-zhu-reddit wants to merge 5 commits into
developfrom
codex/experiments-custom-field-fallback

Conversation

@tim-zhu-reddit

@tim-zhu-reddit tim-zhu-reddit commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

💸 TL;DR

  • Pass untyped context and identifier fields to Rust Decider without adding each name to this library.

📜 Details

  • Preserve typed/extracted-field precedence and isolate per-call overrides.
  • Keep the exported IDENTIFIERS list; accept custom names in all three identifier APIs.
  • Add optional per-call context_fields to get_variant and get_variant_without_expose.
  • Preserve custom exposure values in inputs without colliding with logger metadata or self.
  • Document custom targeting and bucketing usage. No dependency changes.

🧪 Testing Steps / Validation

  • Full suite passes on Python 3.9 with Rust Decider 1.15.0, 1.18.1, and 1.19.0b5.
  • Collision regressions fail before the fixes and pass afterward; six names across six exposure paths.
  • New context_fields tests fail before the change and pass afterward.
  • make lint passes for Python 3.8, 3.9, and 3.10; git diff --check passes; context isolation verified.
  • All seven CI matrix jobs pass at 0bff712.
  • Astra approved the fix delta. Fresh independent Opus review timed out without a verdict.
  • Snoofactor review remains blocked: the integration cannot access this GitHub.com PR.

✅ Checks

  • CI tests (if present) are passing
  • Adheres to code style for repo

Comment thread reddit_decider/__init__.py Outdated
Comment on lines +500 to +521
if identifier_type not in IDENTIFIERS:
logger.warning(
f'"{identifier_type}" is not one of supported "identifier_type": {IDENTIFIERS}.'
)
ctx = self._context_for_identifier(identifier, identifier_type)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah nice! That works great for removing identifier_type restriction for get_variant_for_identifier API.

We may also want to consider expanding the standard get_variant API to take Optional **context_kwargs (similar to **exposure_kwargs) for populating arbitrary fields under other_info dict for the same flexibility as Native-Go Decider.
Since it's optional field, should be backwards compatible.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added in 0bff712

  • get_variant and get_variant_without_expose take an optional keyword-only context_fields dict (python can't have a second **kwargs next to **exposure_kwargs)
  • fields apply to that call only, at the top level and under other_fields, and win over the stored context
  • exposures still log the stored fields plus the bucketing value, same as the identifier APIs

kept it to those two for now. happy to add it to get_all_variants_without_expose and the dynamic config getters too if you want full Native-Go Decider parity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants