feat(ingest): additive schema-v2 write path for test_cases/test_case_runs - #477
Open
ashokponkumar wants to merge 16 commits into
Open
feat(ingest): additive schema-v2 write path for test_cases/test_case_runs#477ashokponkumar wants to merge 16 commits into
ashokponkumar wants to merge 16 commits into
Conversation
…runs v1 tables keep being written exactly as before; this adds the two v2 tables alongside them, guarded on both existing so the script is safe to deploy before the migration lands. One v2 table pair serves all three products, discriminated by a `component` column that replaces the v1 table-name prefix. Both ids are DERIVED, never minted, so this script and the orchestrator agree on the join key with no threading contract -- which is the only thing that makes the tables joinable at all: v1 minted four unrelated identity schemes and artifact_results.run_id consequently joined test_runs.run_id in 2 of 1,266 rows. The recipe must stay byte-identical to spyre-frameworks pipelines/lib/run_identity.py, because an orphaned row is indistinguishable from "no tests ran"; that module's header carries the full rule list and the golden values, and its test suite pins them. run_uid is hashed from the leg's own CI coordinate -- --gha-run-id when GHA dispatched it, else the new --jenkins-run-key -- which is the same value the orchestrator hashes on its side. tags is an Array, not a Map: testtype carries up to 5 values on 91.7% of cases, so a Map would silently keep one and drop the rest. Sorted before hashing. Ingest is idempotent by a skip-if-run_uid-already-present check. That is a hard requirement, not a nicety: test_case_runs is a plain MergeTree with no dedup key and v2 dropped the stored counters because they are derived from these rows, so a double ingest would silently double a leg's counts. Dropped from the v2 path: filename, suite_name, runner_run_id and every stored counter -- all derivable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Ashok Pon Kumar <ashokponkumar@gmail.com>
v2_run_uid already guarded all four of its fields, but v2_test_case_id did not: an empty component or name still hashed to a real, stable uuid, so every such case minted the SAME id and would have collided into one identity rather than merely being orphaned. Rows that collide land looking joined, which is the failure the derived-id design exists to prevent. v2_test_case_id now returns '' without component and name -- the v2 table's CONSTRAINTs reject those as '' regardless -- and insert_v2 skips such a case with a warning instead of writing it. classname stays optional: a module-level test legitimately has none. Matches the same fix to the reference implementation in spyre-frameworks pipelines/lib/run_identity.py, where negative tests are now pinned per field. Goldens unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Ashok Pon Kumar <ashokponkumar@gmail.com>
Brings this writer in line with the schema. The v2 column is run_id -- <thing>_id is the
single rule for every v2 identity (artifact_id, run_id, test_case_id, benchmark_id) -- so
writing run_uid would have inserted a key no table has, silently dropped by
input_format_skip_unknown_fields, leaving every row unjoinable to its per-case detail.
Renamed by word-boundary token, NOT a blind substitution: this file carries BOTH identities.
v1's run_id is a live column and a parameter of insert_run/_runner_run_id (36 occurrences),
and all of those had to survive untouched while the 14 v2 ones changed. Both paths verified
present afterwards.
Verified the hash did not move, only the name: v2_run_id('gha','12345','amd64','integration')
returns dab2a67f-14bf-53be-b6e4-fc9642086e47, byte-identical to
spyre-frameworks pipelines/lib/run_identity.py. A writer that disagrees about the namespace
or field order orphans every row, which is the whole reason these recipes are pinned. arch
still folds (amd64 == x86_64) and an incomplete input is still refused rather than hashed.
Signed-off-by: Ashok Pon Kumar <ashokponkumar@gmail.com>
Mirrors torch-spyre. v2 replaces this repo's hf_test_runs/hf_test_cases/hf_run_properties outright -- run_properties becomes test_cases.tags, and the per-product runs/cases collapse into the SHARED test_cases + test_case_runs keyed by component, which is why the spyre-frameworks v2 DDL deliberately defines neither. This script must keep writing both while the product images turn over, since it runs from a BAKED image and old and new images coexist until every one is rebuilt. --schema v1 (default) | v2 | both, also settable via INGEST_SCHEMA so a workflow sets it once for every leg. Default v1 means an un-updated caller behaves exactly as before; our own invocations pass both. No data is ported: v1 rows cannot produce a v2 run_id, since the hash inputs were never recorded per row. New data only. v2 gets its OWN connection via CLICKHOUSE_DB_V2, not v1's with a different database: test_cases exists in both generations with incompatible shapes, so one database binding cannot serve both. Unset means no second connection is opened, so the default path costs nothing. Gating the inserts alone was not enough -- the v1 dedup READS had to be gated too, since they query the per-product runs table, which need not exist in a v2-only target. Also fixed a pre-existing bug the flag exposed: --platform defaulted to '' here while torch-spyre defaults to the host arch. arch is an INPUT to the run_id hash, so an empty value made v2_run_id refuse to derive an id and every v2 row would have landed unjoinable -- observed as '[warn] v2 skipped: run_id not derivable ... arch=""'. Now defaults to the ingest host's arch, which is the machine the suite ran on. The 'Inserted N test cases' line is gated on write_v1: it reports the PARSE count, so under --schema v2 it claimed inserts that never happened. Verified all three modes against real v1 and v2 databases: default writes v1 only, both writes both, and v2 leaves v1 frozen at 2 runs / 4 cases. The shared v2 tables correctly separate the two producers -- component='hf-adapters' 4 rows, 'spyre-inference' 4 rows. Signed-off-by: Ashok Pon Kumar <ashokponkumar@gmail.com>
Turns on the --schema flag this repo already carries. INGEST_SCHEMA=both, so v1 stays authoritative while v2 accumulates the same runs -- the only way v2 gets history, since no data is ported (v1 rows cannot produce a v2 run_id, the hash inputs were never recorded per row). Set at JOB level rather than on the step: the ingest step has no env: block of its own, and neither step in this workflow shadows CLICKHOUSE_DB_V2, INGEST_SCHEMA or CLICKHOUSE_DB -- checked, not assumed. Safe before the secret exists. An unset CLICKHOUSE_DB_V2 makes get_v2_client() return None and open no second connection at all, so the v2 write is a no-op and this workflow behaves exactly as it does today until the secret is populated. Verified end to end against the real dev database, driven by ENV VARS ONLY with no CLI flag, which is exactly how the workflow invokes it: both repos wrote 2 test_case_runs each into spyre_v2_unified, correctly attributed by component (hf-adapters 2, spyre-inference 2). Probe rows removed afterwards. Signed-off-by: Ashok Pon Kumar <ashokponkumar@gmail.com>
ashokponkumar
marked this pull request as ready for review
September 9, 2026 16:18
The v2 block was in no try/except. v1 is still authoritative and v2 is the experiment, yet any v2-side failure (schema drift, a connection reset, a constraint trip) propagated out of the per-file loop -- aborting it and dropping the v1 insert for every REMAINING xml file in the run. The experiment could take down the source of truth. Wraps the block and continues. The v1 calls stay outside the try, so they are never caught up in a v2 failure. Prerequisite for pointing CLICKHOUSE_DB_V2 at a prod database: with an empty secret the v2 path never runs and the gap is unreachable, so this only becomes load-bearing at the moment the secret is populated. Signed-off-by: Ashok Pon Kumar <ashokponkumar@gmail.com>
ashokponkumar
enabled auto-merge
September 9, 2026 16:35
|
⚙️ Auto-merge enabled — dispatched Automated: enabling auto-merge requests the gate, so the PR is not left waiting on a check nobody asked for. Re-enabling auto-merge on this same commit will not dispatch again. |
❌ spyre-test: failureTriggered by: /spyre-test comment Plan (build waves + dependencies, per arch)amd64 flowchart LR
subgraph Lamd64_0["amd64 L0 · 3 parallel"]
n_amd64_aiu_toolbox_ibm_aiu_toolbox_e2e["aiu-toolbox/ibm-aiu-toolbox-e2e<br/>rpm · b2c90495b6de"]
n_amd64_libaiupti["libaiupti<br/>rpm · ef5890d1ef76"]
n_amd64_spyre_comms["spyre-comms<br/>rpm · c16d3bb366ae"]
end
subgraph Lamd64_1["amd64 L1 · 1 parallel"]
n_amd64_spyre_backend_spyre_backend_dev["spyre-backend/spyre-backend-dev<br/>image · 43e74b455077"]
end
subgraph Lamd64_2["amd64 L2 · 1 parallel"]
n_amd64_torch_spyre_torch_spyre_dev["torch-spyre/torch-spyre-dev 🔴<br/>image · 37239a44c6a5"]
end
subgraph Lamd64_3["amd64 L3 · 1 parallel"]
n_amd64_hf_adapters_hf_adapters_dev["hf-adapters/hf-adapters-dev 🟢<br/>image · 5d853d1a3ac4"]
end
subgraph Lamd64_4["amd64 L4 · 1 parallel"]
n_amd64_spyre_inference_spyre_inference_dev["spyre-inference/spyre-inference-dev 🟢<br/>image · d46fb80cb2ef"]
end
n_amd64_aiu_toolbox_ibm_aiu_toolbox_e2e --> n_amd64_spyre_backend_spyre_backend_dev
n_amd64_libaiupti --> n_amd64_spyre_backend_spyre_backend_dev
n_amd64_spyre_comms --> n_amd64_spyre_backend_spyre_backend_dev
n_amd64_spyre_backend_spyre_backend_dev --> n_amd64_torch_spyre_torch_spyre_dev
n_amd64_torch_spyre_torch_spyre_dev --> n_amd64_hf_adapters_hf_adapters_dev
n_amd64_hf_adapters_hf_adapters_dev --> n_amd64_spyre_inference_spyre_inference_dev
classDef sPending fill:#eceff1,stroke:#90a4ae,color:#37474f
classDef sBuilding fill:#fff8e1,stroke:#f9a825,color:#5d4037,stroke-width:2px
classDef sOk fill:#e8f5e9,stroke:#43a047,color:#1b5e20
classDef sReused fill:#e3f2fd,stroke:#1e88e5,color:#0d47a1
classDef sFailed fill:#ffebee,stroke:#e53935,color:#b71c1c,stroke-width:2px
classDef sDropped fill:#f5f5f5,stroke:#bdbdbd,color:#9e9e9e
class n_amd64_aiu_toolbox_ibm_aiu_toolbox_e2e sOk;
class n_amd64_libaiupti sOk;
class n_amd64_spyre_comms sOk;
class n_amd64_spyre_backend_spyre_backend_dev sOk;
class n_amd64_torch_spyre_torch_spyre_dev sOk;
class n_amd64_hf_adapters_hf_adapters_dev sOk;
class n_amd64_spyre_inference_spyre_inference_dev sOk;
✅ orch trigger-pr-validation — green · arches amd64 · fp amd64=f5e51e92
GHA test runs:
|
Formatting only, from this repo's own pinned ruff. No behaviour change: the v2 write guard is unchanged (verified by AST -- insert_v2 still inside try/except Exception with the v1 calls outside it). Line length differs per repo (88 here vs 100 in spyre-inference), which is why code that passes in one repo fails in another. Signed-off-by: Ashok Pon Kumar <ashokponkumar@gmail.com>
|
GHA test runs:
|
|
GHA test runs:
|
|
GHA test runs:
|
…al lists Every v2 insert was a positional list paired with a separate column_names list, and the same four tables were assembled independently in three repos. Nothing tied a row's field order to the column list except the author reading both. Rows are now dicts keyed by column name and ordered by v2_schema, so the order lives in exactly one place and a field cannot land in the wrong column. This is not a bug fix -- the audited state was correct (20/20 inserts column-named, 17/17 arity right, all four v2 column lists matching the DDL). It removes a class of mistake, and it removes the divergence that let a real defect live in two repos and not the third: hf-adapters and spyre-inference never deduped identity rows ACROSS runs, so a case seen in N runs became N rows in test_cases. insert_identities() is now the only path, so all three get it. Also enforced ahead of the server, where a violation is otherwise a rejected insert that pushJUnitXml swallows into a green build: unknown/missing columns, empty NOT-NULL columns, and the DDL's CONSTRAINT chk_status value set. A v1 column set can no longer be handed to the v2 table -- spyre.test_cases has 14 columns and spyre_v2.test_cases has 6, told apart only by which connection is used. A TABLE model, not an ingester class hierarchy: per-repo variation is one constant (COMPONENT), and the file is COPIED rather than imported because the baked-image path runs `uv run --no-project`, leaving no sys.path beyond the script's own directory and no package to install. check_v2_schema_drift.py keeps the copies honest by comparing parsed ASTs, not bytes -- the repos pin different ruff line lengths, so byte equality is unachievable while semantic equality is what matters. Identity functions are deliberately untouched: run_id and test_case_id arrive already computed. Changing them would re-key the warehouse and silently break v2_already_ingested dedup, producing duplicate rows rather than an error. Verified: the model emits BYTE-IDENTICAL (column_names, rows) pairs to the pre-refactor writer for all three repos on a fixture exercising dedup, tag reordering and a failure -- since only the pair construction changed, pair equality is a complete proof for the insert layer. The pinned identity goldens are unmoved (namespace cb0af9bf..., run_id dab2a67f... and all three per-component test_case_ids). 20 new unit tests pass in each repo; v2_schema imports from the script's own directory, stdlib only. Signed-off-by: Ashok Pon Kumar <ashokponkumar@gmail.com>
Collaborator
Author
|
/spyre-test |
❌ spyre-test: failureTriggered by: /spyre-test comment: #477 (comment) Plan (build waves + dependencies, per arch)amd64 flowchart LR
subgraph Lamd64_0["amd64 L0 · 1 parallel"]
n_amd64_torch_spyre_torch_spyre_dev["torch-spyre/torch-spyre-dev 🔴<br/>image · 41eb35bb4986"]
end
subgraph Lamd64_1["amd64 L1 · 1 parallel"]
n_amd64_hf_adapters_hf_adapters_dev["hf-adapters/hf-adapters-dev 🟢<br/>image · 5b842d804816"]
end
subgraph Lamd64_2["amd64 L2 · 1 parallel"]
n_amd64_spyre_inference_spyre_inference_dev["spyre-inference/spyre-inference-dev 🔴<br/>image · 0fa4eb202ffe"]
end
n_amd64_torch_spyre_torch_spyre_dev --> n_amd64_hf_adapters_hf_adapters_dev
n_amd64_hf_adapters_hf_adapters_dev --> n_amd64_spyre_inference_spyre_inference_dev
classDef sPending fill:#eceff1,stroke:#90a4ae,color:#37474f
classDef sBuilding fill:#fff8e1,stroke:#f9a825,color:#5d4037,stroke-width:2px
classDef sOk fill:#e8f5e9,stroke:#43a047,color:#1b5e20
classDef sReused fill:#e3f2fd,stroke:#1e88e5,color:#0d47a1
classDef sFailed fill:#ffebee,stroke:#e53935,color:#b71c1c,stroke-width:2px
classDef sDropped fill:#f5f5f5,stroke:#bdbdbd,color:#9e9e9e
class n_amd64_torch_spyre_torch_spyre_dev sOk;
class n_amd64_hf_adapters_hf_adapters_dev sOk;
class n_amd64_spyre_inference_spyre_inference_dev sOk;
✅ orch trigger-pr-validation — green · arches amd64 · fp amd64=1844b087
GHA test runs:
|
Three separate failures, all in the new files: license-eye (spyre-inference only -- torch-spyre and hf-adapters have no .licenserc.yaml and no license workflow, verified rather than assumed) rejected all three new files. Added the Apache header its .licenserc.yaml specifies, below the shebang where one exists. ruff in spyre-inference reported 11 errors the other two repos do not: it enforces builtin generics and PEP 604 unions over the typing aliases. Applied the strictest form, so the file now satisfies all three configs at once rather than each repo carrying a different variant. check_v2_schema_drift.py sorts import statements before fingerprinting. Each repo pins its own ruff and their isort rules order `typing` against `collections.abc` differently, so the raw AST differed on files that are otherwise the same code and the check reported false drift. It now compares meaning, insensitive to both formatting and import order -- which is the only thing it was ever meant to assert. Re-verified after the changes: the model still emits BYTE-IDENTICAL (column_names, rows) pairs to the pre-refactor writer in all three repos, the pinned identity goldens are unmoved, 20 tests pass per repo, and the drift check passes across all three copies. Signed-off-by: Ashok Pon Kumar <ashokponkumar@gmail.com>
|
GHA test runs:
|
|
GHA test runs:
|
ruff I001: inserting `import v2_schema` next to the stdlib block left the import block un-sorted. Same one-line fix already applied in torch-spyre and spyre-inference. Scoped to this file deliberately. A repo-wide ruff --fix here also reformats 24 files this branch never touches (hf_adapters/, tests/, evals/, scripts/) -- those findings pre-date this work, this repo's lint was already green, and sweeping them in would hide the v2 change inside an unrelated reformat. They are left alone. Verified: import SET unchanged (only ordering), the model still emits byte-identical (column_names, rows) pairs in all three repos, and the pinned identity goldens are unmoved. Signed-off-by: Ashok Pon Kumar <ashokponkumar@gmail.com>
|
GHA test runs:
|
…directory The ingest tests failed at collection: all 7 in test_ingest_xml and all 12 in test_ingest_kernel_xml errored with ModuleNotFoundError on `import v2_schema`. v2_schema is a SIBLING FILE, not an installed package -- it is copied into three repos and the script is run by path (`uv run --no-project .../ingest_xml*.py`), so the script's own directory is on sys.path only when it is the ENTRY POINT. The tests load it with spec_from_file_location, which does not add that directory, so the import could never resolve there. Fixed in the script rather than the tests: any caller importing this as a module hits the same wall, and the entry-point-only assumption was the real defect. Also records why this file does not use a driver-provided schema layer: clickhouse-connect has none to use. Its ColumnDef is what DESCRIBE TABLE returns -- it reads a live table's schema and cannot declare one or validate a row -- and Client.insert takes "a sequence of sequences" plus an ordered column-name list, so positional rows are the native API shape, not a style choice. column_names='*' only rebinds the order to whatever the server currently reports, coupling every row to the live DDL instead of removing the hazard. clickhouse-sqlalchemy would subsume most of this file, but adding a dependency is what the uv --no-project runtime rules out. Verified: all three scripts now load via spec_from_file_location from an unrelated cwd; 7 + 12 previously-failing ingest tests pass; the model still emits byte-identical (column_names, rows) pairs in all three repos and the pinned identity goldens are unmoved. Signed-off-by: Ashok Pon Kumar <ashokponkumar@gmail.com>
This repo runs black (line-length 88) in ADDITION to ruff; torch-spyre and spyre-inference do not, which is why only this PR failed. I had formatted all three repos with ruff alone, so test_v2_schema.py was left in a shape ruff accepts and black rewrites. Verified black --check, ruff check, ruff format and the hygiene hooks (trailing-whitespace, end-of-file-fixer, debug-statements) all pass together on the new files -- black and ruff can disagree, so satisfying one is not evidence for the other. Cosmetic: 20 tests still pass, the byte-identical (column_names, rows) equivalence holds in all three repos, the pinned identity goldens are unmoved, and the drift check still matches. Signed-off-by: Ashok Pon Kumar <ashokponkumar@gmail.com>
Collaborator
Author
|
/spyre-test |
✅ spyre-test: successTriggered by: /spyre-test comment: #477 (comment) Plan (build waves + dependencies, per arch)amd64 flowchart LR
subgraph Lamd64_0["amd64 L0 · 1 parallel"]
n_amd64_torch_spyre_torch_spyre_dev["torch-spyre/torch-spyre-dev 🟢<br/>image · afd7e2054712"]
end
subgraph Lamd64_1["amd64 L1 · 1 parallel"]
n_amd64_hf_adapters_hf_adapters_dev["hf-adapters/hf-adapters-dev 🟢<br/>image · d08f75a3bd99"]
end
subgraph Lamd64_2["amd64 L2 · 1 parallel"]
n_amd64_spyre_inference_spyre_inference_dev["spyre-inference/spyre-inference-dev 🟢<br/>image · 8d88c2bed8de"]
end
n_amd64_torch_spyre_torch_spyre_dev --> n_amd64_hf_adapters_hf_adapters_dev
n_amd64_hf_adapters_hf_adapters_dev --> n_amd64_spyre_inference_spyre_inference_dev
classDef sPending fill:#eceff1,stroke:#90a4ae,color:#37474f
classDef sBuilding fill:#fff8e1,stroke:#f9a825,color:#5d4037,stroke-width:2px
classDef sOk fill:#e8f5e9,stroke:#43a047,color:#1b5e20
classDef sReused fill:#e3f2fd,stroke:#1e88e5,color:#0d47a1
classDef sFailed fill:#ffebee,stroke:#e53935,color:#b71c1c,stroke-width:2px
classDef sDropped fill:#f5f5f5,stroke:#bdbdbd,color:#9e9e9e
class n_amd64_torch_spyre_torch_spyre_dev sOk;
class n_amd64_hf_adapters_hf_adapters_dev sOk;
class n_amd64_spyre_inference_spyre_inference_dev sOk;
✅ orch trigger-pr-validation — green · arches amd64 · fp amd64=d688c785
GHA test runs:
✅ safe to mergeBuild: built 3 Tests: passed 6 · blocking 0 · advisory 0 · infra/inconclusive 0 · no signal 0 Before merging, consider:
|
|
GHA test runs:
|
|
GHA test runs:
|
|
GHA test runs:
|
Found by ingesting a REAL Spyre-Next run (Spyre-Next/product-test torch-spyre#104, spyre-inference, 7 junit shards) into a dev copy of the prod v2 schema: 9 of 10 cases were silently dropped. v2_already_ingested keyed on (component, run_id). But a sharded run is MANY xml files under ONE run_id -- verified in pushToClickhouse.groovy, which invokes the ingest with --xml-dir and every shard in a single call -- so the first shard's rows made the check true and shards 2..N were skipped as "already ingested". The guard was doing its job for a re-ingest and destroying data for a shard. Now keyed on (component, run_id, props['source_file']). props is in the DDL already and sits outside every key, so recording the filename changes no sort order. Declared on the model's TEST_CASE_RUNS, which is what makes the column writable at all. Double-ingest protection is unchanged: re-running the same 7 shards skips all 7 and leaves exactly 10 rows. Also gated the v1 tables_exist() guard on args.write_v1 (hf/si only). It checks a V1 table, so under --schema v2 an absent v1 schema aborted the whole ingest before the v2 write -- a v2-only database was a silent no-op. Verified against the real data, not a fixture: 7 shards -> 1 run_id / 10 rows / 7 source files; re-ingest -> 7 skipped, still 10 rows; v_run_tier_counters and v_tier_report_completeness both read it correctly. Model still emits byte-identical (column_names, rows) pairs in all three repos, identity goldens unmoved, 22 tests pass per repo. Signed-off-by: Ashok Pon Kumar <ashokponkumar@gmail.com>
anubhavjana
reviewed
Sep 10, 2026
| ) | ||
|
|
||
|
|
||
| def get_v2_client(): |
Collaborator
There was a problem hiding this comment.
Can we use the same get_client()
get_v2_client() opened a SECOND connection bound to the v2 database. It is the same
instance and the same credentials, so the second connection bought nothing that
qualifying the statements does not.
What made it look necessary: test_cases and benchmark_runs exist in BOTH generations
with incompatible shapes (v1 benchmark_runs is run_id UInt64 + source_file, v2 is
run_id UUID + component), so an UNQUALIFIED name resolves against whichever database
the connection holds and silently hits the wrong table. The fix for that is to qualify,
not to open a connection per database.
v2_database() now returns the database NAME, and Table.qualified(db) puts it in front
of the table in every v2 statement. "which database" becomes a property of the CALL
rather than of the connection, which is also why _table_exists() takes an explicit db
instead of asking currentDatabase().
Two things fall out:
- get_client() loses its `database` parameter, which now has no caller passing one.
- v2_new_identity_rows() is deleted; v2_schema.insert_identities() already did the
same cross-run dedup, and the local copy was the only remaining hand-rolled one.
insert_benchmarks_v2() now builds dicts and goes through the schema model like every
other v2 write, so the benchmark tables' column order lives in v2_schema and not in a
positional list paired with its own column_names. BENCHMARKS/BENCHMARK_RUNS gain the
`component` column the DDL already has.
Verified on the live dev instance: a client whose own database is `spyre` wrote and
read spyre_v2_trial through the qualifier alone, v1's table was untouched, and the
identity re-insert was a no-op. The three copies of v2_schema.py pass the drift check.
Signed-off-by: Ashok Pon Kumar <ashokponkumar@gmail.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
2 participants
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.
Draft. Writer-side half of ClickHouse schema v2. Companion PRs: spyre-frameworks
#1517 (the reference
implementation + orchestrator writer) and the two sibling product ingests.
ADDITIVE ONLY. The v1 path is untouched — same tables, same columns, same values, same
dedup. This adds a second write, guarded on both v2 tables existing, so the script is safe to
deploy before the migration lands.
Highest-risk part of the change. Four writers (this script, the two sibling product ingests,
and the orchestrator's
pushToClickhouse.pushArtifactResult) hash independently, with nothreading contract. A writer that disagrees about the namespace, separator, field order or
normalisation mints a different uuid for the same row — and an orphaned row is
indistinguishable from "no tests ran", so the failure is completely silent.
Rules, all load-bearing: separator
|; fields positional, never sorted; each field.strip().lower();archfolded (amd64/x86/x86-64→x86_64) inside the hash, notleft to the caller;
tagsdeduped, sorted,,-joined.Golden values (pinned in
spyre-frameworks pipelines/lib/test_run_identity.py):run_uid("gha","123","x86_64","regression")1a6080e8-d061-547f-ab63-1af99b18ad0ctest_case_id("torch-spyre","test_ops","test_add",["testtype__trunk","platform__x86_64"])2f0e2626-3b33-56c7-9019-fb261450c7aasourceis required and separates colliding id spaces (a GHA run id and a Jenkins buildnumber share a number space).
external_run_idis a String — typedUInt64every Jenkinsleg would be
0and collide into one identity.Why derived rather than threaded
Measured on prod:
artifact_results.run_idjoinstest_runs.run_idin 2 of 1,266 rows,and the perf tables in 0 of 34. v1 minted four unrelated schemes (orchestrator uuid per
row, ingest uuid4 per XML file, perf-suite random UInt64, vLLM GHA workflow id) — so nothing
joined.
run_uidis computed from values each writer already holds, which is what removes thecross-job contract that a retry could break.
run_uidcomes from the leg's own CI coordinate:--gha-run-idwhen GHA dispatched it, elsethe new
--jenkins-run-key(the leg's externalizable id, e.g.Spyre/component-build#417) —the same value the orchestrator hashes on its side. With neither, it falls back to
source='local'so the rows stay self-consistent, just not linked to an artifact.tagsis anArray, not aMaptesttypecarries up to 5 values on 91.7% of cases, so aMapwould silently keep one anddrop the rest — destroying the tier-subset filtering (
integration ⊂ regression ⊂ trunk) thev2 reuse logic depends on. Sorted before hashing.
Idempotency is a hard requirement
test_case_runsis a plainMergeTreewith no dedup key, and v2 dropped the storedcounters precisely because they are derived from these rows. So a double ingest of one leg
would silently double its counts. Implemented as a skip-if-
run_uid-already-present check,scoped by
componentto hit theORDER BYprefix.Dropped from the v2 path as specified:
filename,suite_name,runner_run_id, and everystored counter.
Verified by execution (dev ClickHouse, database
spyre_v2_ingest_test, since dropped)4 cases → 4
test_cases+ 4test_case_runs; all 4 join back.arch=amd64and this script's givenarch=x86_64produced the samerun_uid, andartifact_results ⋈ test_case_runs ⋈ test_casesreturned 4 of 4 cases.run_uid/test_case_idfor identical inputs across the reference module and all three ingest scripts.testtypesurvives the round trip;has(tags,'testtype__trunk')filterscorrectly; derived counters match the parsed suite.
distinct
test_case_ids.--gha-run-id/--jenkins-run-key/ neither all resolve as intended, and a missing--trigger-typeproduces a blankrun_uidplus a loud warning rather than anall-defaults uuid every incomplete leg would share.
Not verified by execution: no real CI workflow has run this script — the callers do not yet
pass
--jenkins-run-key, so until they do, Jenkins-dispatched legs take thelocalfallbackand their rows will not join an artifact.
Blocker found, needs a decision on #1516
v1
spyre.test_casesand v2test_casesare different tables with the same name (v1:run_id/case_id/op_name/dtype/triggered_at/…; v2:test_case_id/component/tags).The v2 DDL was validated against an empty database, so this never surfaced. In a database
holding both, whichever exists wins and the other writer fails — I hit exactly that: the
v1
insert_caseserrored withUnrecognized column 'run_id' in table test_cases.Additive coexistence therefore is not possible in one database as currently named. v2 needs
either a separate database or a distinct table name. This is a schema decision, not a writer
change, so it is not addressed here — but it blocks cutover, and it is the reason my
verification ran the two paths against separate databases.
Correction (follow-up commit): the blank-on-missing-field guard was documented but absent
An earlier version of this PR body stated "blank return (not a hash) when a field is missing".
That was false for the reference module and is now fixed.
team-leadcaught it bymeasurement; recording it here because the failure mode is instructive.
The Groovy writer and all three ingest scripts did guard
run_uid. The one place missing itwas
pipelines/lib/run_identity.py— the module that is supposed to define the rule. Andv2_test_case_idin the ingest scripts did not guard at all.Why it is worse than an orphan: an empty field still hashes to a real, stable uuid, so every
incomplete leg sharing the remaining fields mints the same id.
Unrelated runs merge, and the rows land looking joined — the exact failure the derived-id
design exists to prevent. An orphan is at least visibly missing.
Fixed:
run_uidreturnsNoneunless all four fields are non-empty after normalisation;test_case_idreturnsNonewithoutcomponentandname(the v2CONSTRAINTs reject thoseas
''regardless;classnamestays optional — a module-level test legitimately has none), andinsert_v2skips such a case with a warning rather than writing it.Negative tests are pinned per field, for empty / whitespace-only /
None, plus a positivetest reproducing a real stored id. Per-field is deliberate: the guard existed for three of four
fields in some writers and none in others, so a single "missing input is refused" test would
have passed while the hole stayed open. Suite is now 28 tests + 12 subtests.
Goldens unchanged — both pinned values and
run_uid("jenkins","Spyre-Next/product-test#89","s390x","unit")=
a0669f2b-3358-5ccb-8aa5-67bf8955a3c0still reproduce.Process note worth more than the bug: this claim was asserted by two agents independently and
verified by neither. It was in a PR body, a docstring and a commit message before anyone ran it.
Everything else in these PRs was verified by execution; this one line was not.