feat(python-sdk): add deterministic bucketing and rule evaluation core - #1
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request lays the foundational groundwork for the Python SDK by porting the critical deterministic core logic, specifically focusing on visitor bucketing and rule evaluation, directly from the existing JavaScript SDK. The primary goal of this initial phase is to achieve exact behavioral parity in these high-risk areas, ensuring a consistent and reliable base. Higher-level features, such as public SDK entry points, context management, API interactions, and data handling, are intentionally deferred to subsequent pull requests, allowing for a focused and robust implementation of the core decision-making mechanisms. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request establishes the foundational Python SDK for Convert, implementing core functionalities such as deterministic MurmurHash3-based bucketing and a robust rule management system with various comparison methods. The changes include comprehensive project setup, modular SDK structure, and initial unit tests to ensure parity with existing JavaScript behavior. The review identified several areas for optimization and code quality improvement, including enhancing the efficiency of float-to-integer truncation and list/set lookups in utility functions, caching method lists in the rule manager, addressing a typo in an enum, and refactoring a duplicated dataclass to adhere to the DRY principle.
| experience_id = str(options.get("experienceId", "")) | ||
| hash_value = generate_hash(experience_id + str(visitor_id), seed) | ||
| value = (hash_value / DEFAULT_MAX_HASH) * self._max_traffic | ||
| return int(str(value).split(".", 1)[0]) |
There was a problem hiding this comment.
This method of truncating a float to an integer is inefficient and not idiomatic Python. It involves converting the number to a string, splitting it, and then converting it back to an integer. A direct conversion using int() is much cleaner and more performant.
| return int(str(value).split(".", 1)[0]) | |
| return int(value) |
|
|
||
|
|
||
| class BucketingError(str, Enum): | ||
| VARIAION_NOT_DECIDED = "convert.com_variation_not_decided" |
There was a problem hiding this comment.
There is a typo in the enum member name VARIAION_NOT_DECIDED. It should be VARIATION_NOT_DECIDED.
| VARIAION_NOT_DECIDED = "convert.com_variation_not_decided" | |
| VARIATION_NOT_DECIDED = "convert.com_variation_not_decided" |
| def get_comparison_processor_methods(self) -> list[str]: | ||
| return [ | ||
| name | ||
| for name, value in self._comparison_processor.items() | ||
| if callable(value) | ||
| ] |
There was a problem hiding this comment.
This method recalculates the list of method names on every call. Since it's used inside loops during rule processing (_process_rule_item), this can be inefficient. It's better to cache this list and invalidate the cache only when the comparison_processor is updated.
To implement this, you'll need to:
- Add
self._comparison_methods_cache: list[str] | None = Noneto__init__. - Invalidate the cache in the
comparison_processorsetter by settingself._comparison_methods_cache = None. - Update this method to use the cache.
| def get_comparison_processor_methods(self) -> list[str]: | |
| return [ | |
| name | |
| for name, value in self._comparison_processor.items() | |
| if callable(value) | |
| ] | |
| def get_comparison_processor_methods(self) -> list[str]: | |
| if self._comparison_methods_cache is None: | |
| self._comparison_methods_cache = [ | |
| name | |
| for name, value in self._comparison_processor.items() | |
| if callable(value) | |
| ] | |
| return self._comparison_methods_cache |
| @dataclass(frozen=True) | ||
| class BucketingAllocationType: | ||
| variation_id: str | ||
| bucketing_allocation: int |
There was a problem hiding this comment.
The BucketingAllocationType dataclass appears to be a duplicate of BucketingAllocation defined in src/convertcom_sdk/bucketing/bucketing_manager.py. To adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability, this type should be defined in a single location (e.g., here in types.py) and imported wherever it's needed.
| if object_not_empty(value): | ||
| return _apply_negation( | ||
| str(test_against) in [str(key) for key in value.keys()], | ||
| negation, | ||
| ) |
There was a problem hiding this comment.
Creating a new list of all keys converted to strings for every check ([str(key) for key in value.keys()]) can be inefficient for large dictionaries. Using a generator expression with any() is more memory-efficient as it avoids creating the full list in memory.
| if object_not_empty(value): | |
| return _apply_negation( | |
| str(test_against) in [str(key) for key in value.keys()], | |
| negation, | |
| ) | |
| if object_not_empty(value): | |
| return _apply_negation( | |
| any(str(test_against) == str(key) for key in value), | |
| negation, | |
| ) |
| def is_in( | ||
| values: str | int | float, | ||
| test_against: list[str | int | float] | str, | ||
| negation: bool = False, | ||
| splitter: str = "|", | ||
| ) -> bool: | ||
| matched_values = [str(item) for item in str(values).split(splitter)] | ||
| if isinstance(test_against, str): | ||
| candidates: list[Any] = test_against.split(splitter) | ||
| elif isinstance(test_against, list): | ||
| candidates = test_against | ||
| else: | ||
| candidates = [] | ||
| normalized_candidates = [str(item).lower() for item in candidates] | ||
| for item in matched_values: | ||
| if item in normalized_candidates: | ||
| return _apply_negation(True, negation) | ||
| return _apply_negation(False, negation) |
There was a problem hiding this comment.
The check if item in normalized_candidates: is performed inside a loop. Since normalized_candidates is a list, this check has a time complexity of O(N), where N is the number of candidates. For a large number of candidates, this can be inefficient. Converting normalized_candidates to a set would make the lookups average O(1) time complexity.
| def is_in( | |
| values: str | int | float, | |
| test_against: list[str | int | float] | str, | |
| negation: bool = False, | |
| splitter: str = "|", | |
| ) -> bool: | |
| matched_values = [str(item) for item in str(values).split(splitter)] | |
| if isinstance(test_against, str): | |
| candidates: list[Any] = test_against.split(splitter) | |
| elif isinstance(test_against, list): | |
| candidates = test_against | |
| else: | |
| candidates = [] | |
| normalized_candidates = [str(item).lower() for item in candidates] | |
| for item in matched_values: | |
| if item in normalized_candidates: | |
| return _apply_negation(True, negation) | |
| return _apply_negation(False, negation) | |
| def is_in( | |
| values: str | int | float, | |
| test_against: list[str | int | float] | str, | |
| negation: bool = False, | |
| splitter: str = "|", | |
| ) -> bool: | |
| matched_values = (str(item) for item in str(values).split(splitter)) | |
| if isinstance(test_against, str): | |
| candidates: list[Any] = test_against.split(splitter) | |
| elif isinstance(test_against, list): | |
| candidates = test_against | |
| else: | |
| candidates = [] | |
| normalized_candidates = {str(item).lower() for item in candidates} | |
| for item in matched_values: | |
| if item in normalized_candidates: | |
| return _apply_negation(True, negation) | |
| return _apply_negation(False, negation) |
All eight open Dependabot alerts on this repo sit in `yarn.lock` — the dev-only semantic-release tooling. None of them reach the published wheel/sdist (httpx is still the only runtime dependency). Three of the four highs were already reachable inside the existing semver ranges; the fourth (sigstore GHSA-52v5-jr5w-gjxr, `certificateOIDs` verification constraints silently dropped) needed sigstore >= 4.1.1, which only arrives through a major bump: semantic-release 24.2.9 -> 25.0.9 @semantic-release/npm 12.0.2 -> 13.1.5 npm 10.9.8 -> 11.19.0 libnpmpublish 10 -> 11.2.0 }-> sigstore ^3 -> ^4 (4.1.1) pacote 19 -> 21.5.1 } @sigstore/core 2.0.0 -> 3.2.1 tar 7.5.16 -> 7.5.22 `@semantic-release/github` moves to ^12 to match what semantic-release 25 depends on — leaving it at ^11 would hoist the older copy to the project root and shadow the one core resolves. Resolved (4 high, 4 medium — no criticals were open): #14 high ip-address 10.2.0 -> 10.4.0 (needs >= 10.3.1) #9 high brace-expansion 2.1.1 -> 5.0.9 (needs >= 2.1.2) #7 high js-yaml 4.2.0 -> 4.3.1 (needs >= 4.3.0) #2 high sigstore 3.1.0 -> 4.1.1 (needs >= 4.1.1) #13 medium ip-address (same bump as #14) #12 medium ip-address (same bump as #14) #4 medium tar 7.5.16 -> 7.5.22 (needs >= 7.5.18) #1 medium @sigstore/core 2.0.0 -> 3.2.1 (needs >= 3.2.1) Node: semantic-release 25 requires ^22.14.0 || >= 24.10.0. release.yml installs `lts/*`, currently Node 24.19.0 — satisfied, and every future LTS line stays above the floor. Verified: `yarn install --immutable` (what release.yml runs) passes against the regenerated lockfile with the lockfile format unchanged (__metadata version 10), and `yarn npm audit --all --recursive` reports no suggestions. A `semantic-release --dry-run` against this branch loads all four configured plugins, passes verifyConditions for both exec and github (GitHub authentication + push permission), and analyzes commits to "no release" — correct, since `chore` is a non-releasing type in release.config.mjs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Summary
This PR creates the initial Python SDK implementation scaffold and ports the deterministic core behavior from the JavaScript SDK. The focus of this PR is parity-critical logic that must match the JS SDK before we move on to higher-level managers and the public SDK surface.
What’s Included
python-sdkpackage scaffold with packaging/dev setupequalsequalsNumbermatcheslesslessEqualcontainsisInstartsWithendsWithregexMatchesRuleManagerwith the same OR -> AND -> OR_WHEN traversal model as the JS SDKBucketingManagerwith deterministic visitor-based bucket selectionParity Work Completed
murmurhashpackage used by the JS SDKnumbertypeVerification
41 passedOut of Scope For This PR
This PR intentionally does not include:
ConvertSDK/ public SDK entrypointContextApiManagerDataManagerSegmentsManagerExperienceManagerFeatureManagerThose will follow in the next PRs once the deterministic core is locked down.
Why This Order
Bucketing and rule evaluation are the highest-risk areas for parity. Getting these right first ensures that later layers are built on top of behavior that already matches the JavaScript SDK.