SK-2954: common/v2/v3 split + flowvault insert support#269
Open
saileshwar-skyflow wants to merge 3 commits into
Open
SK-2954: common/v2/v3 split + flowvault insert support#269saileshwar-skyflow wants to merge 3 commits into
saileshwar-skyflow wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Semgrep OSS found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
…lowdb (v3) insert support Restructures the repo into three build variants sharing a bundled common/ module (SK-2938 Option C): v2 (today's SDK, behavior-preserving) and a new v3 built on the flowservice/flowdb API, insert-only this round. common/ - Shared credential resolution, vault-URL resolution, and bearer-token fetch/cache/expiry logic (VaultController, BaseVaultClient), enums, errors, service_account, and generic validators. - VaultController declares insert/get/update/delete/query/detokenize as abstract methods (Java-interface-style); v2 and v3 each provide their own concrete/stub implementations. v2 - Relocated from the repo root via git mv; public API unchanged (same class names, signatures, import paths). Vault is now a backward-compatible alias for the internal PdbVaultController class. - Fixed a latent bug where v2's own Env enum failed cross-class comparisons against common's Env; both now share one definition. v3 (skyflow-flowvault, starting at 1.0.0) - New InsertRequest/InsertRecord/Upsert/InsertResponse types, FlowVaultController, and VaultClient targeting the flowservice REST API. - Insert validation ported from Java's v3 Validations.java: table/upsert must live in exactly one place (request-level or per-record, matching in both), 10k record cap, empty key/value checks. - InsertResponse mirrors Java's v3 shape (summary/success/errors) as plain dicts, each result tagged with its index in the original record list (stable across batch boundaries). - Structured per-record error parsing from the backend's actual error body, plus x-request-id propagation onto error entries. - Batching via INSERT_BATCH_SIZE (default 50, max 1000), sequential, isolate-and-continue on a failing batch. - Vault URL resolution uses v3's own skyvault.skyflowapis.* domain for all four envs (DEV/SANDBOX/STAGE/PROD), confirmed to differ from v2's vault.skyflowapis.* domain. CI/CD - shared-tests.yml and shared-build-and-deploy.yml now take a `variant` input and scope every step to v2/ or v3/ via working-directory. - main.yml, ci.yml, beta-release.yml, internal-release.yml, and release.yml matrix over both variants. v3 releases are distinguished from v2's via a flowvault- tag/branch prefix (flowvault-1.0.0, flowvault-release/*) so a release trigger is never ambiguous between the two independently-versioned packages; v2's existing bare-semver tags are untouched. - Fixed ruff.toml/.codespellrc still excluding a pre-split "skyflow/generated" path that no longer existed after the relocation. - Fixed a bump_version.sh sed collision with a comment that happened to contain the literal text "__version__ = ...". - Added a common/ test job to main.yml/ci.yml. Note: v3/samples/ and the root samples/ folder are deliberately excluded from this branch/commit -- local working copies there contain credentials used for live testing against a real vault and must not be pushed. Tests: common 36, v3 65, v2 426 (2 pre-existing unrelated fixture failures), tests/contract passing for both variants. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
57c72e0 to
cadf5d4
Compare
Distribution name was already skyflow-flowvault (setup.py) but the importable package stayed skyflow, colliding with v2's skyflow import name if both are ever installed in the same environment. Rename the v3 directory to flowvault and its package to skyflow_flowvault to match the distribution name and remove the collision. generated/ content is left untouched (Fern-owned).
…d base insert response Consolidates duplicated logic between v2 (PDB) and flowvault per architecture review: shared validation (vault config, credentials, log level), LogLevel/Logger, and insert field/table validation now live in common with per-variant message injection; adds BaseInsertResponse alongside BaseInsertRequest so each variant's InsertRequest/InsertResponse can extend a common base while keeping its own shape. Also fixes flowvault's insert() response shape (drop redundant 'data'/'table', flatten tokens, errors=None when empty) and a stale SDK_VERSION drift bug. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Splits skyflow-python from a single-variant SDK into three build variants sharing a bundled
common/module (SK-2938 Option C, matching the pattern already used in skyflow-java's own v2/v3 split):common/— shared credential resolution, vault-URL resolution, bearer-token fetch/cache/expiry, enums, errors, and generic validators. Never published independently; bundled into each variant's wheel at build time.v2/— today's SDK, relocated from the repo root. Public API unchanged — same class names, signatures, import paths.v3/— new, built on the flowservice/flowdb API. Insert-only this round by design; every other operation is deliberately out of scope until a follow-up round. Publishes asskyflow-flowvault, starting at1.0.0.common/VaultControllerabstract base (mirrors Java'sVaultControllerinterface): declaresinsert/get/update/delete/query/detokenizeas abstract methods. v2'sPdbVaultControllerand v3'sFlowVaultControllereach provide their own concrete/stub implementations — v2 already had all six; v3 only implementsinsertfor real, with explicit "not implemented yet" stubs for the rest so it satisfies the abstract contract.BaseVaultClientabstract base: shared credential/bearer-token/vault-URL-resolution logic, withresolve_vault_url/initialize_api_clientas per-variant hooks (v2 and v3 vaults live on different subdomains —vault.skyflowapis.*vsskyvault.skyflowapis.*— confirmed against a real vault).v2/Vaultis kept as a backward-compatible alias for the new internalPdbVaultControllername, sofrom skyflow.vault.controller import Vaultand everything downstream of it keeps working unchanged.Envenum was failing cross-class comparisons (env not in Env) againstcommon'sEnvbecause they were two structurally-identical-but-distinct enum classes.v2/skyflow/utils/enums/env.pyis now a re-export ofcommon's.v3/— insert supportInsertRequest/InsertRecord/Upsert(Java-parity "rich" shape — table/upsert can be set at the request level or overridden per-record).InsertResponsemirrors Java's v3 shape (summary/success/errors) as plain dicts — no custom response classes — with every result tagged with itsindexin the original record list (stable across batch boundaries, so callers can correlate a result back viarequest.records[result['index']]).EnvUrlsconfirmed (DEV/SANDBOX/STAGE/PROD, each onskyvault.skyflowapis.*), with test coverage for each.v3/.../utils/validations/Validations.java, not reinvented:datadict keys/values can't be null or empty.recordslist) into individual error entries instead of one flat message per failed batch, and propagatesx-request-idonto each error for support/debugging. Verified against a real vault's actual 400 response (a NOT NULL column violation).INSERT_BATCH_SIZEenv var (default 50, max 1000), sequential (no concurrency this round), isolate-and-continue — a failing batch doesn't abort the rest.Skyflow/Builderclient facade andFlowVaultController/VaultClientwired to the flowservice REST client.commonis never part of the public interface —skyflow.error/skyflow.utilsre-export what consumers need (SkyflowError,Env,LogLevel), matching v2's own established shim pattern.CI/CD
Every existing workflow (
main,ci,beta-release,internal-release,release, and the two shared reusable workflows) now works for both variants instead of just v2:shared-tests.yml/shared-build-and-deploy.ymltake avariantinput and scope every build/test/bump/publish step tov2/orv3/viaworking-directory.main.yml/ci.ymlmatrix over both variants (plus a newcommon/-only test job, since it now contains real shared logic worth gating on).beta-release.yml/internal-release.yml/release.ymlmatrix over both variants with a job-level dispatch. v3 releases use aflowvault-tag/branch prefix (flowvault-1.0.0,flowvault-release/*) so a release trigger is never ambiguous between the two independently-versioned packages — v2's existing bare-semver tags (2.1.3,release/*) are completely untouched.ruff.toml/.codespellrcwere still excluding askyflow/generatedpath that stopped existing once the repo split (now excludes baregenerated, matching at any depth); andbump_version.sh's version-bump sed had a collision with a code comment that happened to contain the literal text it was matching on.Testing
common/tests— 36 tests, new.v3/tests— 65 tests, new (validation parity, wire-shape mapping, batching, response splitting, error parsing, header injection, all fourEnvUrls).v2/tests— 426 tests, zero modifications to the existing suite (2 pre-existing failures are an unrelated gitignoredcredentials.jsonfixture, present before this change).tests/contract/— one shared insert() contract asserted against both variants' own installed environment.Known follow-ups (not done here, intentionally out of scope)
get/update/delete/query/detokenizeare stubs (NotImplementedError) — planned for follow-up rounds, one operation at a time, same pattern as this round's insert.flowvault-tag/branch push since some of this couldn't be dry-run outside GitHub's own infrastructure.