feat: support sdk key and direct config initialization - #9
Conversation
There was a problem hiding this comment.
Code Review
This pull request establishes the core initialization framework for the Convert Python SDK, including configuration management, validation, and immutable domain snapshots. It introduces an HTTPX-based transport adapter and a typed error hierarchy. The review feedback suggests enhancing documentation with error-handling examples, using explicit assertions for type safety in the configuration loader, and removing redundant validation calls.
| core = Core( | ||
| SDKConfig( | ||
| sdk_key="1001/2002", | ||
| environment="staging", | ||
| transport=TransportConfig( | ||
| config_endpoint="https://cdn-4.convertexperiments.com/api/v1", | ||
| ), | ||
| ) | ||
| ) | ||
| assert core.is_ready |
There was a problem hiding this comment.
The example for initializing with an SDK key is helpful. To make it more robust and production-ready for users, consider demonstrating how to handle potential initialization failures. Since this involves a network request, it could fail and raise an InitializationError. Wrapping the call to Core() in a try...except block would be a great addition.
For example:
from convert_sdk import Core, SDKConfig, TransportConfig, InitializationError
try:
core = Core(
SDKConfig(
sdk_key="1001/2002",
# ...
)
)
assert core.is_ready
print("SDK initialized successfully!")
except InitializationError as e:
print(f"SDK initialization failed: {e}")| request = ConfigRequest( | ||
| sdk_key=config.sdk_key or "", | ||
| sdk_key_secret=config.sdk_key_secret, | ||
| environment=config.environment, | ||
| transport=config.transport, | ||
| ) |
There was a problem hiding this comment.
While config.sdk_key or "" works to satisfy the type checker, it's less clear than an explicit assertion. Since validate_sdk_config is called just before this, we can be sure that config.sdk_key is not None. Using an assert makes this assumption explicit, which improves readability and helps static analysis tools.
| request = ConfigRequest( | |
| sdk_key=config.sdk_key or "", | |
| sdk_key_secret=config.sdk_key_secret, | |
| environment=config.environment, | |
| transport=config.transport, | |
| ) | |
| assert config.sdk_key is not None | |
| request = ConfigRequest( | |
| sdk_key=config.sdk_key, | |
| sdk_key_secret=config.sdk_key_secret, | |
| environment=config.environment, | |
| transport=config.transport, | |
| ) |
| if has_config_data: | ||
| validate_config_data(config.config_data) |
There was a problem hiding this comment.
The load_config_snapshot function already calls validate_config_data when config.config_data is present. This call within validate_sdk_config is redundant and causes the validation to be performed twice. It's best to keep validation concerns separate and let the orchestrator (load_config_snapshot) handle the sequence of validation calls.
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
Implements Story 1.2, "Support sdkKey and Direct-Config Initialization," for the Python SDK.
This PR adds the first real SDK initialization behavior on top of the Story 1.1 scaffold. The SDK can now initialize from either:
sdk_keyfetched from the Convert config endpoint over HTTPSIt also establishes the immutable config snapshot foundation that later stories will use for context creation and evaluation.
What Changed
SDKConfigTransportConfigInitializationErrorConfigValidationErrorConfigLoadErrorCoreto initialize from either direct config data orsdk_keyconvert_sdk.config_loaderConfigSnapshotcreation with precomputed entity indexeshttpx-backed sync transport adaptersdk_keyinitializationhttpxruntime dependencyWhy
The planning artifacts for Story 1.2 call for the SDK to support both preloaded and networked initialization modes while keeping the MVP public API sync-first and framework-agnostic.
This PR establishes:
CoreThat gives later stories a stable base for visitor contexts, evaluation, and tracking.
Validation
Ran in
../python-sdk:uv sync --group devuv run pytest -p no:cacheprovideruv buildResults:
Notes
This PR is intentionally limited to initialization and config ingestion.
It does not yet implement: