Use:feat(python-sdk): add core, context, and public sdk surface - #4
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 significantly advances the Python SDK by integrating core managers into a new public API surface, comprising 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 introduces a new, more structured SDK architecture by adding Core, Context, and ConvertSDK classes, along with configuration management, new entity types, and improved event and data handling for bucketing and conversions. The review identified several areas for improvement: the update_visitor_properties method in Context has a potentially confusing visitor_id parameter, there are opportunities to optimize list comprehensions in run_feature and run_features for efficiency and readability, and the lookup for EntityType.VARIATION in get_config_entity could be made more performant. Additionally, the on_ready method in Core should be renamed for clarity, and a typo in the VISITOR_ID_REQUIRED error message needs correction.
| def update_visitor_properties(self, visitor_id: str, visitor_properties: Mapping[str, Any]) -> None: | ||
| self._data_manager.put_data(visitor_id, {"segments": dict(visitor_properties)}) |
There was a problem hiding this comment.
The update_visitor_properties method on the Context class accepts a visitor_id. This is confusing and potentially error-prone, as a Context instance is already associated with a specific visitor (self._visitor_id). This design could lead to unintentionally modifying data for a different visitor from within a specific visitor's context.
To improve clarity and prevent misuse, this method should operate on the visitor associated with the current context. I recommend removing the visitor_id parameter and using self._visitor_id instead. The aliased method updateVisitorProperties should also be updated to reflect this change in signature.
| def update_visitor_properties(self, visitor_id: str, visitor_properties: Mapping[str, Any]) -> None: | |
| self._data_manager.put_data(visitor_id, {"segments": dict(visitor_properties)}) | |
| def update_visitor_properties(self, visitor_properties: Mapping[str, Any]) -> None: | |
| self._data_manager.put_data(self._visitor_id, {"segments": dict(visitor_properties)}) |
| for item in [feature for feature in items if isinstance(feature, Mapping)]: | ||
| self._event_manager.fire( | ||
| SystemEvents.BUCKETING, | ||
| { | ||
| "visitorId": self._visitor_id, | ||
| "experienceKey": item.get("experienceKey"), | ||
| "featureKey": key, | ||
| "status": item.get("status"), | ||
| }, | ||
| None, | ||
| True, | ||
| ) |
There was a problem hiding this comment.
The list comprehension [feature for feature in items if isinstance(feature, Mapping)] creates a new temporary list just for iteration. This is slightly inefficient and can be made more readable by using a simple for loop with a check inside.
| for item in [feature for feature in items if isinstance(feature, Mapping)]: | |
| self._event_manager.fire( | |
| SystemEvents.BUCKETING, | |
| { | |
| "visitorId": self._visitor_id, | |
| "experienceKey": item.get("experienceKey"), | |
| "featureKey": key, | |
| "status": item.get("status"), | |
| }, | |
| None, | |
| True, | |
| ) | |
| for item in items: | |
| if isinstance(item, Mapping): | |
| self._event_manager.fire( | |
| SystemEvents.BUCKETING, | |
| { | |
| "visitorId": self._visitor_id, | |
| "experienceKey": item.get("experienceKey"), | |
| "featureKey": key, | |
| "status": item.get("status"), | |
| }, | |
| None, | |
| True, | |
| ) |
| for item in [feature for feature in bucketed_features if isinstance(feature, Mapping)]: | ||
| self._event_manager.fire( | ||
| SystemEvents.BUCKETING, | ||
| { | ||
| "visitorId": self._visitor_id, | ||
| "experienceKey": item.get("experienceKey"), | ||
| "featureKey": item.get("key"), | ||
| "status": item.get("status"), | ||
| }, | ||
| None, | ||
| True, | ||
| ) |
There was a problem hiding this comment.
Similar to run_feature, the list comprehension here creates an unnecessary temporary list. A simple for loop with a check would be more efficient and readable.
| for item in [feature for feature in bucketed_features if isinstance(feature, Mapping)]: | |
| self._event_manager.fire( | |
| SystemEvents.BUCKETING, | |
| { | |
| "visitorId": self._visitor_id, | |
| "experienceKey": item.get("experienceKey"), | |
| "featureKey": item.get("key"), | |
| "status": item.get("status"), | |
| }, | |
| None, | |
| True, | |
| ) | |
| for item in bucketed_features: | |
| if isinstance(item, Mapping): | |
| self._event_manager.fire( | |
| SystemEvents.BUCKETING, | |
| { | |
| "visitorId": self._visitor_id, | |
| "experienceKey": item.get("experienceKey"), | |
| "featureKey": item.get("key"), | |
| "status": item.get("status"), | |
| }, | |
| None, | |
| True, | |
| ) |
| if entity_type == EntityType.VARIATION: | ||
| for experience in self._data_manager.get_entities_list("experiences"): | ||
| variation = self._data_manager.get_sub_item( | ||
| "experiences", | ||
| experience.get("key"), | ||
| "variations", | ||
| key, | ||
| "key", | ||
| "key", | ||
| ) | ||
| if variation: | ||
| return variation | ||
| return None |
There was a problem hiding this comment.
When entity_type is EntityType.VARIATION, this method iterates through all experiences to find a matching variation. This can be inefficient if there are many experiences, leading to O(N) complexity where N is the number of experiences.
For better performance, consider optimizing the data structure within DataManager to allow for a more direct lookup of variations, for example by pre-processing the config data to create a map of variation keys/IDs to their parent experience.
| def on_ready(self) -> None: | ||
| if self._data_manager.data: | ||
| return None | ||
| raise ValueError(DATA_OBJECT_MISSING) |
There was a problem hiding this comment.
The method on_ready is named in a way that suggests it's for registering a callback, similar to on(). However, its implementation is a synchronous check that raises an exception if the SDK is not ready. This can be misleading for developers, especially given the goal of JS-style compatibility where onReady often implies an asynchronous operation or callback registration.
Consider renaming this method to better reflect its behavior, for example assert_ready() or check_ready().
| RULE_DATA_NOT_VALID = "Rule data is not valid" | ||
| SDK_OR_DATA_OBJECT_REQUIRED = "SDK key or Data object should be provided" | ||
| DATA_OBJECT_MISSING = "Data object is missing" | ||
| VISITOR_ID_REQUIRED = "Visitor string string is not present" |
There was a problem hiding this comment.
There's a typo in the error message for VISITOR_ID_REQUIRED. It says "Visitor string string is not present".
| VISITOR_ID_REQUIRED = "Visitor string string is not present" | |
| VISITOR_ID_REQUIRED = "Visitor ID string is not present" |
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>
This PR adds the public Python SDK surface by wiring the previously ported managers into
ConvertSDK,Core, andContext. With this PR, the Python SDK can now initialize from static config orsdkKey, create visitor contexts, evaluate experiences/features, run custom segments, track conversions, and expose config/visitor helper methods.What’s Included
ConvertSDK,Core, andContextrun_experiencerun_experiencesrun_featurerun_featurestrack_conversionset_default_segmentsrun_custom_segmentsupdate_visitor_propertiesget_config_entityget_config_entity_by_idget_visitor_datarelease_queuescreateContextrunExperiencerunFeaturestrackConversiononReadyrefreshConfigDataManagerto support:EntityTypeand supporting error/config exportsBehavior Added
This PR now enables:
sdkKeyvia sync config fetchTesting
Added public-surface tests covering:
sdkKeyconfig fetch initializationVerification:
.venv/bin/pytest -q69 passedOut of Scope
This PR does not yet cover broader production hardening work such as:
Those should come in the next follow-up PR.