Vendor databricks-dbt-factory v0.3.2 (core + tests) into the dbt-factory example and template - #2
Conversation
…rop duplicated core tests - Add scripts/vendor_dbt_factory.sh: copies the CLI-stripped core subset from a pinned upstream tag into the template and example (byte-identical). - Re-vendor the core from v0.3.1 (readable keys, FQN selectors, pure DbtFactory). - Glue: select serverless notebook tasks via task_type=TaskType.NOTEBOOK. - Drop the vendored core-logic tests (test_dbt_factory/test_utils/test_dbt_task); the core is a verbatim release artifact tested upstream. Keep test_load_resources (the PyDABs glue integration). - Exclude the template's vendored core from the repo ruff check (formatted at the library's line-length, like the other bundle templates).
- Bump both NOTICE files: vendored from v0.3.1 (c8755f7d), verbatim minus the excluded CLI files; drop the stale trimmed/reformatted wording. - Remove the dead snapshot-refresh Make target and the README/tree references to the deleted core tests, in both example and template.
New workflow: (1) asserts the example and template vendored cores are byte-identical (re-run scripts/vendor_dbt_factory.sh to sync); (2) runs the example's PyDABs integration tests. Actions pinned to full SHAs per the repo policy.
…ger CI on itself - vendor_dbt_factory.sh: correct the post-run notes (NOTICE 'Vendored from' line, run the example tests) and note the ruff pin must match fmt.yml. - Add test_vendored_core.py (both copies): unconditional smoke test that the vendored core is the pinned version and generates notebook tasks, so a missing manifest can't skip the suite to a false green. - dbt-factory-sync.yml: also trigger on edits to the workflow itself.
mwojtyczka
left a comment
mwojtyczka
left a comment
There was a problem hiding this comment.
Automated code-review findings from /code-review. Two runtime regressions (🔴) carried in by the re-vendor, plus two coverage gaps (🟡).
mwojtyczka
left a comment
mwojtyczka
left a comment
There was a problem hiding this comment.
Changes for the actual good are mainly good. Left a few comments. I would put the tests back and make sure the CI catches discrepancies with the vendored code.
- Re-vendor the core from v0.3.2 (notebook runner host/isolation fixes; notebook is now the default task type). - Vendor the upstream core tests (conftest, test_utils, test_dbt_task, and test_dbt_factory minus its job_spec-dependent golden-spec tests) into both the example and template, so the bundle's test coverage does not depend on the upstream repo. Added scripts/vendor_test_dbt_factory.py for the test_dbt_factory adaptation, and taught vendor_dbt_factory.sh to vendor the tests. - CI now diffs the vendored test files between example and template too, and the example test run exercises the vendored core logic. - NOTICE and version pin updated to v0.3.2.
park-peter
changed the title
The PyDABs glue test is duplicated byte-identical across the example and template but was not in the sync diff, so a one-sided edit could drift silently.
Exclude the template copies of test_vendored_core.py and test_load_resources.py from repo ruff: they must stay byte-identical to the example copies (which resolve to line-length 120), but the template has no pyproject so they fell to the default 88 — a code line 89-120 chars would make the sync guard unsatisfiable. Also guard NOTICE for example/template drift, add the version-pin bump to the re-vendor runbook, and make the test transform fail loudly if an expected import line is not present.
mwojtyczka
left a comment
mwojtyczka
left a comment
There was a problem hiding this comment.
Automated code-review findings (re-review of the v0.3.2 re-vendor). The two prior 🔴 runtime regressions are resolved by v0.3.2 — these remaining items are lower-severity: vendoring mechanics, drift-guard gaps, and dead/thinned test coverage. Verified against head 749d561.
|
|
||
| echo "Vendoring core and tests into template and example ..." | ||
| for root in "$TEMPLATE_ROOT" "$EXAMPLE_ROOT"; do | ||
| rm -rf "$root/src/databricks_dbt_factory" |
There was a problem hiding this comment.
🟡 Re-vendor can leave stale test files behind. rm -rf "$root/src/databricks_dbt_factory" fully cleans the core dir before recopying, but the tests loop below (line 86) only overwrites files by explicit name. If a future upstream tag removes one of the vendored test files, the old copy persists in both the example and template identically — so the byte-equality sync guard still passes while both silently drift from upstream. Consider rm -rf on the vendored test set (or the whole staged subtree) before copying, symmetric with how src/ is handled.
| if ! diff -r --exclude=__pycache__ "$template/src/databricks_dbt_factory" "$example/src/databricks_dbt_factory"; then | ||
| ok=1 | ||
| fi | ||
| for f in conftest.py test_dbt_factory.py test_dbt_task.py test_utils.py test_vendored_core.py test_load_resources.py; do |
There was a problem hiding this comment.
🟡 Sync check omits tests/__init__.py. This hardcoded list diffs conftest.py + the test_*.py files (plus the src/ dir and NOTICE), but not tests/__init__.py, which exists in both copies. If it ever diverges between example and template, the guard reports in-sync while they differ — exactly the drift this workflow exists to catch. (Broader note: a hand-maintained file list is itself drift-prone; a diff -r over the whole tests/ dir, or a reproducibility check that re-runs the vendor script and asserts no git diff, would be self-maintaining.)
| @@ -1,552 +0,0 @@ | |||
| [ | |||
There was a problem hiding this comment.
🟡 Exact-output snapshot of the example's generated notebook tasks is deleted. expected_tasks.json + update_expected_tasks.py are removed. The retained vendored tests run through conftest.create_dbt_factory, which defaults to task_type='dbt', so the notebook-task rendering path the example actually uses (TaskType.NOTEBOOK, base_parameters, project/profiles dirs) is only loosely covered by test_load_resources.py's shape assertions. A change that silently alters the concrete generated notebook output (task key, --select FQN, base_parameters) would no longer be caught by an exact snapshot.
|
closing for now, will do as a follow up PR |
2 participants
What this does
Re-vendors the
databricks-dbt-factorycore (and its core tests) into thedbt-factoryexampleand template from the released v0.3.2 tag, and sets up mechanical re-sync so future updates
are a one-command, CI-verified step instead of a hand-merge.
Targets the
dbt-factory-contribbranch so it folds into PR databricks#163.Changes
scripts/vendor_dbt_factory.sh— clones a pinned upstream tag, stages + formats theCLI-stripped core subset and the upstream core tests, and writes them into both the template
and the example, leaving the two byte-identical. The core subset excludes the upstream CLI files
(
main.py,job_spec.py) the bundle never uses.hand-adaptation. Relative to the previously-vendored v0.2.1 this brings in readable collision-safe
task keys, full-FQN
--select(fixes duplicate-name and subdirectory-model selection), dbtunit-test handling, the notebook-runner host/isolation fixes, and notebook as the default task
type.
test coverage):
conftest.py,test_utils.py,test_dbt_task.pyverbatim, andtest_dbt_factory.pyadapted viascripts/vendor_test_dbt_factory.py— its fourjob_spec-dependent golden-spec tests are dropped (the bundle doesn't vendorjob_spec.py),keeping the ~21 DAG/gating tests. Also kept:
test_load_resources.py(PyDABs glue integration)and
test_vendored_core.py(version pin).task_type=TaskType.NOTEBOOK)..github/workflows/dbt-factory-sync.yml) — fails if the example and templatevendored core or test files drift, and runs the example test suite.
.ruff.tomlexcludes the template's vendored files (formatted at the library's line-length).
Effect on generated output
The generated job's structure is unchanged — same tasks, same
depends_onDAG, samenotebook-task shape. Two things in the generated output change, both from the upstream fixes:
orders_raw_run→orders_raw_model.--selectuses the full FQN:--select orders_raw→--select dbt_factory.example.orders_raw(the bare name was ambiguous across packages and matched nothing for models in subdirectories).
Consequence: redeploying an existing job renames every task, so per-task run history keyed by
the old names won't carry over.
Addressing review feedback
v0.3.2; carried in by this re-vendor.
keys,
depends_onwiring, and command text against the vendored core, so a future re-vendorthat silently alters output fails CI.
example and template, not just the core dir. (The
bundle initrender step itself is still notCI-exercised, consistent with the other templates in this repo.)
DbtTaskdual-mode / notebook default — notebook is now the default task type upstream inv0.3.2, aligning the library, the CLI, and this bundle.
Verification
ruff format --check .clean from repo root.vendor_dbt_factory.sh … v0.3.2run.load_resourcesagainst the committed manifest generates 5 serverless notebook tasks.