Conversation
Run all three CI jobs on pull requests and pushes to main, replacing the scheduled SQL Server workflow so the job that would break a change can actually gate it. Close the oracle gap left by deleting the C# HL7 importer. Three more frozen outputs under contracts/golden/ carry the importer-boundary, OBR-date-fallback, and blank-narrative assertions that had no golden, and a README records that these files are database snapshots rather than envelopes -- naaccr_value.value_num is a REAL column, while the envelope's values[].value_num is the source lexeme as a string. A test asserts both representations so neither can be reconciled into the other. Stop --dry-run from writing: the SQL Server backend no longer creates schemas at construction, and the SQLite backend opens existing files read-only and substitutes in-memory databases for absent ones. Settle the sdc_form_answer column contract. `response` holds the raw source lexeme for every typed answer and `datatype` names which typed column holds the parsed value, so both importers agree and no answered question has a null response. Numeric values now parse with TryParse, so one malformed value no longer discards the whole submission. Drop the vocabulary loader's allowance for the eight seed concepts that only the deleted InsertEssentialConcepts() ever produced, along with the tests that seeded them. Rename NAACRToOMOPmaps to NAACCRtoOMOPmaps and delete phenoml-workflows, amending REBUILD_PLAN.md, which had assigned that deletion to Phase 4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
I asked Copilot to summarize this PR, and will copy it here. Next step after this comment is the code review.Here’s the user-focused summary of PR #97 (IHE-SDC-WG/SDC-CDM) in plain language. This PR is a major simplification and reset of the project’s first build phase so it is easier to run, verify, and maintain. If you’re using this software, the key outcome is: Why it matters to users From a user perspective, this PR is about reliability and clarity: Biggest changes you’ll notice
The codebase is reorganized into: This makes it clearer where to look depending on language and role. A Python build driver now applies database schema changes in a defined order for these areas: It supports SQLite and SQL Server, and is intended to be rerunnable. User impact: running setup/build is more repeatable and less fragile. C# paths for HL7, FHIR, and vocabulary-stub logic were removed in this phase. User impact: less ambiguity about supported ingestion in current phase; focus is on SDC XML path. PostgreSQL-specific dialect/container wiring was removed from active flow. User impact: current tested path is focused on SQLite + SQL Server only in this PR’s scope. A contracts/ area is introduced with: These preserve expected behavior previously covered by removed tests. User impact: easier to confirm that outputs are still in the expected shape over time. Three CI jobs are now configured for PRs and pushes to main. User impact: higher confidence that main workflows remain healthy before/after merges. For sdc_form_answer: Also includes: User impact: more predictable data interpretation and safer test runs. Size / risk profile (for review planning) This is a large PR: So this is primarily a restructure + scope reduction + build/contract foundation PR, not a small feature tweak. If you want, next I can give you a review checklist focused on user-facing risk (what could break for someone running imports/builds), without diving into low-level implementation details yet. -------------More RM questions: --------------------------- What CI checks are run on PR and Push? Remember to examine these questions and the status report model when I later ask you to review the PR. Great set of questions. I can answer them accurately, but I need to pull the PR’s actual file diffs/check runs/comments first so I don’t guess. I’ll do that next and then give you: And yes — I’ll keep this as the baseline model for your later formal PR review. ---------------------Mr AI needs a litle push--------------------------
|
There was a problem hiding this comment.
Pull request overview
Restructures the repository into a Phase 0 “skeleton” with a manifest-driven Python database build, a migration ledger for safe re-runs, a reduced C# surface limited to SDC XML persistence, and CI that validates SQLite + SQL Server builds—while removing the PostgreSQL dialect and related wiring.
Changes:
- Added Python
sdc_cdm buildCLI with manifest validation, SQL script splitting, SQLite/SQL Server backends, and a migration ledger to make builds idempotent and rerunnable. - Reworked the database model to five logical schemas (
etl,intake,omop,naaccr,sdc), updated DDL/docs/diagrams accordingly, and removed PostgreSQL DDL + container setup from active paths. - Trimmed C# to SDC XML template/row-data import + SQLite store only, updated solution/projects/tests, and added GitHub Actions CI for Python + C# + SQL Server.
Reviewed changes
Copilot reviewed 126 out of 135 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/tests/test_three_schema_sqlite.py | Removes PostgreSQL/Docker bootstrap assertions; updates column rename contract assertion. |
| tools/tests/test_load_athena_vocab.py | Drops PostgreSQL backend coverage and seed-concept behavior; updates rollback expectations. |
| tools/tests/test_convert_naaccr_omop_maps.py | Updates default workbook directory rename (NAACCRtoOMOPmaps). |
| tools/requirements-vocab.txt | Removes PostgreSQL driver dependency (psycopg). |
| tools/load_athena_vocab.py | Removes PostgreSQL backend, tightens “fresh target” requirements, updates bridge-required concept handling. |
| tools/convert_naaccr_omop_maps.py | Updates mapping-workbook default directory and removes phenoml-workflows boundary language. |
| tools/ccr_labreport_to_naaccr.py | Updates CLI description to “five-schema store”. |
| src/python/tests/test_sqlscript.py | Adds tests for SQLite/SQL Server script splitting and pins executable unit counts for manifest entries. |
| src/python/tests/test_omop_ddl.py | Adds SQL Server OMOP DDL identity-column regression test. |
| src/python/tests/test_no_postgres.py | Enforces that removed-dialect text is confined to declared reference paths. |
| src/python/tests/test_migration_ledger.py | Adds unit tests for migration decision policy (skip/reapply/accept-hash/mismatch). |
| src/python/tests/test_manifest.py | Adds manifest ordering + exclusion validation tests. |
| src/python/tests/test_contract.py | Adds envelope schema strictness checks and golden snapshot assertions. |
| src/python/tests/test_build.py | Adds CLI/build idempotency + dry-run immutability tests and schema file layout checks. |
| src/python/sdc_cdm/vocab/init.py | Placeholder package for Phase 1 vocabulary support. |
| src/python/sdc_cdm/hl7v2/init.py | Placeholder package for Phase 3 HL7 v2 parsing support. |
| src/python/sdc_cdm/export/init.py | Placeholder package for Phase 5 export support. |
| src/python/sdc_cdm/envelope/init.py | Placeholder package for Phase 3 envelope support. |
| src/python/sdc_cdm/db/sqlserver_backend.py | Implements SQL Server backend (pyodbc) for manifest-driven builds. |
| src/python/sdc_cdm/db/sqlscript.py | Adds dialect-aware SQL splitting utilities (SQLite transaction stripping, SQL Server GO batching). |
| src/python/sdc_cdm/db/sqlite_backend.py | Implements attached-database SQLite backend with read-only dry-run behavior. |
| src/python/sdc_cdm/db/run_log.py | Adds etl.run logging for build runs (SQLite + SQL Server semantics). |
| src/python/sdc_cdm/db/paths.py | Adds repo path helpers (repo root + manifest path). |
| src/python/sdc_cdm/db/manifest.py | Adds manifest loader/validator enforcing ordering and complete DDL coverage/exclusions. |
| src/python/sdc_cdm/db/ledger.py | Adds migration ledger + hash-change policy with --accept-changed-hashes. |
| src/python/sdc_cdm/db/errors.py | Adds stable error types for CLI exit semantics. |
| src/python/sdc_cdm/db/backend.py | Defines backend interface used by build driver. |
| src/python/sdc_cdm/db/init.py | Package marker for DB tooling. |
| src/python/sdc_cdm/cli/main.py | Implements sdc-cdm CLI with build command and dry-run/list modes. |
| src/python/sdc_cdm/cli/build.py | Implements manifest application, ledger decisions, and run logging. |
| src/python/sdc_cdm/cli/init.py | Package marker for CLI entrypoints. |
| src/python/sdc_cdm/main.py | Enables python -m sdc_cdm entrypoint. |
| src/python/sdc_cdm/init.py | Defines Python package metadata/version. |
| src/csharp/SdcCdm.sln | Renames solution projects to SdcCdm.Sdc + SdcCdm.Sdc.Tests and removes retired projects. |
| src/csharp/SdcCdm.Sdc/SdcCdm.Sdc.csproj | New trimmed C# library project with embedded SDC SQLite DDL resource. |
| src/csharp/SdcCdm.Sdc/ISdcCdm.cs | New trimmed interface contract reflecting SDC-only API surface and response_string rename. |
| src/csharp/SdcCdm.Sdc/ImportTemplateRowData.cs | Imports template row data CSV into SDC SQLite store (new implementation). |
| src/csharp/SdcCdm.Sdc/ImportTemplate.cs | Imports SDC template XML metadata into store (new implementation). |
| src/csharp/SdcCdm.Sdc.Tests/TemplateRowDataImporterTests.cs | Adds C# test verifying template row-data import creates expected template metadata. |
| src/csharp/SdcCdm.Sdc.Tests/SdcCdm.Sdc.Tests.csproj | Updates test data inclusion and references trimmed C# project only. |
| src/csharp/README.md | New C# scope documentation: SDC XML only, no pipeline/bridge/SQL Server store. |
| SdcCdmLib/SdcCdmInSqlite/TestImport.cs | Removes legacy manual test harness executable. |
| SdcCdmLib/SdcCdmInSqlite/SdcCdmInSqlite.csproj | Removes retired SQLite implementation project. |
| SdcCdmLib/SdcCdm/SdcCdm.csproj | Removes retired “core” C# library project (HL7/FHIR/vocab stubs). |
| SdcCdmLib/SdcCdm/ISdcCdm.cs | Removes retired expansive interface and the misspelled response column API. |
| SdcCdmLib/SdcCdm/ImportXmlForm.cs | Removes retired XML form importer implementation from old library. |
| SdcCdmLib/SdcCdm/ImportCsv.cs | Removes retired CSV vocab importer stub. |
| SdcCdmLib/SdcCdm/File/Handler.cs | Removes retired helper enum used by deleted FHIR parser. |
| SdcCdmLib/SdcCdm/FHIR/Parse.cs | Removes retired FHIR parsing helper. |
| SdcCdmLib/SdcCdm/FHIR/Importers.cs | Removes retired FHIR importer stubs. |
| SdcCdmLib/SdcCdm/FHIR/Converters.cs | Removes retired FHIR→OMOP converter stub. |
| SdcCdmLib/SdcCdm.Tests/VocabImporterTests.cs | Removes retired C# vocab importer tests. |
| SdcCdmLib/SdcCdm.Tests/FhirCpdsExporterTests.cs | Removes retired FHIR exporter tests and redundant template-row-data test. |
| SdcCdmLib/README.md | Removes outdated documentation for retired C# library layout. |
| sample_data/ecp_query_examples.sql | Updates comments to reflect five-schema model and removed PostgreSQL. |
| README.md | Rewrites top-level README for manifest-driven build and five-schema layout; documents tool support and testing. |
| pyproject.toml | Adds Python package definition, optional deps, scripts entrypoint, and pytest configuration. |
| notebooks/serve_db.py | Updates Datasette-lite URL generation for five attached SQLite schema files. |
| notebooks/README.md | Removes .NET notebook section and updates attached-file documentation for five-schema build. |
| notebooks/python_cdm_utils/import_vol_v_message_sqlite.py | Adds provenance commit reference in docstring (legacy utility). |
| notebooks/python_cdm_utils/import_sdc_form_sqlite.py | Updates response/typed-column contract (response + datatype + response_string). |
| notebooks/python_cdm_utils/crud_sqlite.py | Renames reponse_string_nvarchar to response_string in helper API/SQL. |
| ECP_OMOP_MAPPING.md | Updates prose for five-schema model and Phase boundaries (no active HL7 importer in Phase 0). |
| diagrams/three-schema/three-schema-overview.mmd | Updates ERD overview for five schemas and new intake/etl relationships. |
| diagrams/three-schema/sdc.mmd | Documents response/datatype contract and renames to response_string. |
| diagrams/three-schema/naaccr-sdc-to-omop-bridge.mmd | Updates bridge notes to reflect Phase 0 execution approach and removed Postgres text. |
| diagrams/README.md | Updates diagrams guidance and adds etl/intake source-of-truth DDL references. |
| database/vocab/README.md | Updates vocab-loading instructions for manifest build; removes PostgreSQL section and adjusts dialect guidance. |
| database/update-ddl-files.py | Removes tombstone script (retired generator guard). |
| database/schemas/sdc/ddl/sqlserver/1_sdc_sqlserver_ddl.sql | Renames reponse_string_nvarchar to response_string. |
| database/schemas/sdc/ddl/sqlite/1_sdc_sqlite_ddl.sql | Renames reponse_string_nvarchar to response_string. |
| database/schemas/sdc/ddl/postgresql/1_sdc_postgresql_ddl.sql | Removes PostgreSQL SDC DDL (dialect removed). |
| database/schemas/omop/VENDORED.md | Adds provenance notes and local modification rules for vendored OMOP DDL. |
| database/schemas/omop/ddl/sqlserver/1_OMOPCDM_sqlserver_5.4_ddl.sql | Applies SQL Server identity/type adjustments required for rerunnable builds and bridge assumptions. |
| database/schemas/naaccr/ddl/sqlserver/0_naaccr_dictionary_sqlserver.sql | Updates dialect text (“both supported dialects”). |
| database/schemas/naaccr/ddl/sqlite/1_naaccr_sqlite_ddl.sql | Updates dialect text (“both supported dialects”). |
| database/schemas/naaccr/ddl/postgresql/1_naaccr_postgresql_ddl.sql | Removes PostgreSQL NAACCR DDL (dialect removed). |
| database/schemas/intake/ddl/sqlserver/1_intake_sqlserver_ddl.sql | Adds SQL Server intake schema DDL (inbound_message, diagnostics, patient). |
| database/schemas/intake/ddl/sqlite/1_intake_sqlite_ddl.sql | Adds SQLite intake schema DDL for attached database layout. |
| database/schemas/etl/ddl/sqlserver/1_etl_sqlserver_ddl.sql | Adds SQL Server etl schema DDL (run log, migration ledger, constants). |
| database/schemas/etl/ddl/sqlite/1_etl_sqlite_ddl.sql | Adds SQLite etl schema DDL for ledger + run log. |
| database/README.md | Updates database docs to five schemas and documents manifest-driven build workflow. |
| database/naaccr_omop/README.md | Updates mapping workbook directory rename and removes phenoml-workflows review guidance. |
| database/naaccr_omop/naaccr_omop_extension_mapping_spec.json | Updates phenoml-workflows boundary string and workbook path renames. |
| database/manifest.json | Adds ordered per-dialect manifest plus explicit excluded reference files. |
| database/Dockerfile | Removes PostgreSQL container init wiring. |
| database/docker-compose.yml | Removes PostgreSQL docker-compose wiring. |
| database/.env.example | Removes PostgreSQL docker env example. |
| contracts/SERIALIZATION.md | Adds canonical envelope JSON serialization profile. |
| contracts/golden/README.md | Documents frozen “golden” importer outputs and their provenance. |
| contracts/golden/obx-Adrenal.obr_date_fallback.json | Adds frozen snapshot for OBR-date fallback behavior. |
| contracts/golden/obx-Adrenal.naaccr_value.json | Adds frozen snapshot for naaccr_value outputs. |
| contracts/golden/obx-Adrenal.measurement.json | Adds frozen snapshot for bridged measurement outputs. |
| contracts/golden/obx-Adrenal.importer_boundary.json | Adds frozen snapshot asserting importer boundary (no SDC XML form tables). |
| contracts/golden/24-11-000312-2.sdc_report.json | Adds frozen snapshot covering blank narrative fallback behavior. |
| .gitignore | Removes ignores related to deleted phenoml-workflows outputs/venv. |
| .github/workflows/sqlserver.yml | Adds SQL Server CI job with mssql service, ODBC driver install, and pytest run. |
| .github/workflows/ci.yml | Adds Python SQLite + C# SDC jobs and enforces no cross-language dependency execution. |
| phenoml-workflows/workflows/naaccr-to-omop.workflow.json | Removes phenoml-workflows workflow definition (package deleted). |
| phenoml-workflows/sample/naaccr-case.example.json | Removes phenoml-workflows sample input (package deleted). |
| phenoml-workflows/requirements.txt | Removes phenoml-workflows dependencies list. |
| phenoml-workflows/README.md | Removes phenoml-workflows documentation. |
| phenoml-workflows/pyproject.toml | Removes phenoml-workflows packaging metadata. |
| phenoml-workflows/phenoml_workflows/run_workflow.py | Removes phenoml-workflows CLI harness. |
| phenoml-workflows/phenoml_workflows/review_store.py | Removes phenoml-workflows review store implementation. |
| phenoml-workflows/phenoml_workflows/review_schema.py | Removes phenoml-workflows review schema helpers. |
| phenoml-workflows/phenoml_workflows/mapper.py | Removes phenoml-workflows mapping logic. |
| phenoml-workflows/phenoml_workflows/json_io.py | Removes phenoml-workflows JSON IO utilities. |
| phenoml-workflows/phenoml_workflows/config.py | Removes phenoml-workflows credential/config handling. |
| phenoml-workflows/phenoml_workflows/init.py | Removes phenoml-workflows package marker. |
| phenoml-workflows/.env.example | Removes phenoml-workflows env example. |
| notebooks/try_sdc_cdm_dotnet.dib | Removes .NET polyglot notebook. |
| .vscode/launch.json | Removes editor launch configuration for deleted .NET console target. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Concepts the NAACCR-to-OMOP bridge joins against. Checked in the Athena extract, | ||
| # not in the target database, so a bundle missing them fails before it is loaded. | ||
| BRIDGE_REQUIRED_CONCEPT_IDS = frozenset({0, 32817, 32879, 1147289}) |
| A fresh manifest build leaves the OMOP vocabulary tables empty. The loader also accepts the | ||
| small known bridge-seed set from older databases and replaces those rows with the canonical | ||
| Athena records. |
Restructures the repo into
src/python+src/csharpand adds a manifest-driven Python build driver whose migration ledger applies the ordered DDL for five schemas (etl,intake,omop,naaccr,sdc) to SQLite and SQL Server, sobuildis safely rerunnable. Trims C# to SDC XML import only — deleting the HL7, FHIR, and vocabulary-stub code — and removes the PostgreSQL dialect and its container wiring, keeping the vendored OHDSI files present but unapplied. Addscontracts/: the versioned envelope schema, its serialization profile, and five frozen importer outputs that preserve what the deleted C# HL7 tests asserted, plus three CI jobs running on pull requests and pushes tomain. Settles thesdc_form_answercolumn contract (responseholds the raw source lexeme,datatypenames which typed column holds the parsed value), renamesreponse_string_nvarchartoresponse_string, guards numeric parsing withTryParse, and makes--dry-rungenuinely read-only on both dialects. Also renamesNAACRToOMOPmapstoNAACCRtoOMOPmapsand deletesphenoml-workflows, brought forward from Phase 4 withREBUILD_PLAN.mdamended to match.Closes #90. Follow-ups noted on #91, #93, #94, and #96.
🤖 Generated with Claude Code