feat(elt-common): Add transform functionality - #427
Conversation
ref #421 Transform 'jobs' are going to be based off of the ingest job names, so it turns out there isn't really a need for them to have manifests.
Most of elt-common is no longer dlt based, so it seems like a good time to update this
📝 WalkthroughWalkthroughChangesThe PR adds dbt transformation execution to ELT orchestration
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
elt-common/src/elt_common/transform.py (1)
11-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd pytest coverage for the transform execution flow.
Mock
dbtRunner,run_ingest, andrun_transform. Assert the dbt selector, remote-profile arguments, step order forall, transform-only execution, and non-success exit handling.
elt-common/src/elt_common/transform.py#L11-L27: Test command construction anddbtRunner.invoke()arguments.elt-common/src/elt_common/cli.py#L58-L86: Testall,ingest, andtransformstep selection.elt-common/src/elt_common/cli.py#L110-L131: Test transform failure reporting and exit status.As per coding guidelines, write unit and e2e tests for the
elt-commonpackage usingpytestand place them underelt-common/tests/.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@elt-common/src/elt_common/transform.py` around lines 11 - 27, Add pytest unit and end-to-end coverage under elt-common/tests/: for elt-common/src/elt_common/transform.py lines 11-27, mock dbtRunner and verify dbt selector construction, remote profile arguments, and dbtRunner.invoke() inputs; for elt-common/src/elt_common/cli.py lines 58-86, mock run_ingest and run_transform and verify step selection and ordering for all, ingest-only, and transform-only flows; for elt-common/src/elt_common/cli.py lines 110-131, verify non-success transform results are reported and produce the expected exit status.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@elt-common/src/elt_common/transform.py`:
- Around line 11-27: Add pytest unit and end-to-end coverage under
elt-common/tests/: for elt-common/src/elt_common/transform.py lines 11-27, mock
dbtRunner and verify dbt selector construction, remote profile arguments, and
dbtRunner.invoke() inputs; for elt-common/src/elt_common/cli.py lines 58-86,
mock run_ingest and run_transform and verify step selection and ordering for
all, ingest-only, and transform-only flows; for elt-common/src/elt_common/cli.py
lines 110-131, verify non-success transform results are reported and produce the
expected exit status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d676ad60-0504-473a-8049-121d84c1fcab
⛔ Files ignored due to path filters (2)
elt-common/uv.lockis excluded by!**/*.lockelt-pipelines/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
elt-common/README.mdelt-common/pyproject.tomlelt-common/src/elt_common/cli.pyelt-common/src/elt_common/extract.pyelt-common/src/elt_common/ingest.pyelt-common/src/elt_common/pipeline.pyelt-common/src/elt_common/pipeline_types.pyelt-common/src/elt_common/transform.pyelt-common/tests/unit_tests/test_extract.pyelt-common/tests/unit_tests/test_ingest.pyelt-common/tests/unit_tests/test_pipeline.py
Closes #421
ELTJobManifest->ELTIngestManifestandrunner.py->ingest.pytransformsteps with dbt viaelt runI've tested this locally with the
electricity_sharepointpipeline (using #403), with the entirety of thewarehouses/facility_ops/transformdirectory copied intoelt-pipelines, because the DB schema in the landing warehouse hasn't changed.elt run facility_ops electricity_sharepointorelt run facility_ops electricity_sharepoint --step transformto only run the new step.Couple of notes:
source:selector to avoid relying on the names of the modelsingest,transform, andrun(which is ingest then transform) - rather than justrunwith the--stepoption. No real opinion on which is better.Follow up work:
facility_ops/transformfromwarehousestoelt-pipelinestransformdirectory/dbt projectremoteargument, to allow this to actually run against prod. This'll require docs about how the profiles in the dbt project need to be set up.