Add compatibility with dbt v1.10 in run_results
#161
Conversation
WalkthroughUpdates the run results v6 parser: adds Metadata.invocation_started_at (optional), changes default Metadata.dbt_version to "1.10.0a1", and extends Status enum with no_op = "no-op". Also bumps package version to "0.10.1". No control-flow changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🔇 Additional comments (3)
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 |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
dlouseiro
changed the title
run_resultsrun_results, catalog and sources
dlouseiro
force-pushed
the
dlouseiro/add-support-for-dbt-1.10-in-run-results
branch
from
fee37fd to
afa58db
Compare
|
Hey @yu-iskw , sorry for bothering you, but would be great to have some feedback on this PR and ideally have it merged/deployed as we're currently blocked to upgrade some of our processes to dbt Thank you! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
dbt_artifacts_parser/parsers/manifest/manifest_v12.py (3)
741-743: Prefer typed enum default over string.Use UpdatesOn.any to avoid string coercion and improve type clarity.
class BuildAfter(BaseParserModel): @@ - updates_on: Optional[UpdatesOn] = 'any' + updates_on: Optional[UpdatesOn] = UpdatesOn.any
2620-2626: Repeat: prefer enum default over string in BuildAfter1.class BuildAfter1(BaseParserModel): @@ - updates_on: Optional[UpdatesOn] = 'any' + updates_on: Optional[UpdatesOn] = UpdatesOn.any
30-34: Remove patch-specific dbt_version default; prefer None for artifact valueVerified: many generated parser modules set hardcoded, per-version dbt_version values (manifest_v12.py: dbt_version='1.10.8', manifest_v11.py:'1.8.0a1', manifest_v9.py:'1.5.0b5', run_results_v6.py:'1.10.0a1', sources_v3.py:'1.10.0a1', catalog_v1.py:'1.10.0a1', etc.). These are patch-specific and will drift — set dbt_version: Optional[str] = None (or omit the default) in manifest_v12.py (and align other generated parsers) so the artifact-provided value is used.
dbt_artifacts_parser/parsers/sources/sources_v2.py (1)
20-20: Prefer default None for generated_at to avoid a misleading static timestamp.A concrete default like
'2021-09-24T13:29:14.312598Z'can mask missing data. UsingNonebetter reflects optionality.Proposed change:
- generated_at: Optional[AwareDatetime] = '2021-09-24T13:29:14.312598Z' + generated_at: Optional[AwareDatetime] = NonePlease double‑check downstream code doesn’t rely on the existing non‑None default.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
dbt_artifacts_parser/resources/manifest/manifest_v12.jsonis excluded by!**/*.json
📒 Files selected for processing (3)
dbt_artifacts_parser/parsers/manifest/manifest_v12.py(12 hunks)dbt_artifacts_parser/parsers/run_results/run_results_v5.py(2 hunks)dbt_artifacts_parser/parsers/sources/sources_v2.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
dbt_artifacts_parser/parsers/run_results/run_results_v5.py (2)
dbt_artifacts_parser/parsers/base.py (1)
BaseParserModel(23-26)dbt_artifacts_parser/parsers/run_results/run_results_v4.py (1)
BaseArtifactMetadata(15-23)
dbt_artifacts_parser/parsers/manifest/manifest_v12.py (1)
dbt_artifacts_parser/parsers/base.py (1)
BaseParserModel(23-26)
🔇 Additional comments (18)
dbt_artifacts_parser/parsers/manifest/manifest_v12.py (12)
732-735: UpdatesOn enum addition — LGTM.
750-751: Required build_after: verify backward compatibility with existing v12 manifests.Making build_after mandatory inside Freshness can reject older manifests lacking it. If this is intentional for dbt ≥1.10 only, all good; otherwise consider keeping it optional.
1459-1466: Freshness1 shape for sources — LGTM.
1546-1553: Freshness2 variant — LGTM.
1562-1567: Source config fields added — LGTM; thanks for covering gaps.Matches the intent to add loaded_at_* and metadata fields.
Please confirm that the same fields exist in the sources artifact parser to keep parity.
1589-1590: Sources.freshness reference — LGTM.
2629-2633: Freshness3 with required build_after — LGTM (see earlier BC note).Ensure manifests produced by your targeted dbt versions include build_after when this Freshness variant is present.
2668-2669: Config33.freshness wiring — LGTM.
3313-3320: Freshness4 variant — LGTM.
3390-3396: Freshness5 variant — LGTM.
3405-3410: Disabled source config: added freshness + loaded_at_ + meta/tags — LGTM.*
3432-3433: Disabled8.freshness reference — LGTM.dbt_artifacts_parser/parsers/sources/sources_v2.py (4)
64-65: LGTM: TimingInfo now uses timezone‑aware datetimes.Consistent with the rest of the artifact changes; no issues spotted.
82-83: Confirm requiredness of max_loaded_at/snapshotted_at across supported dbt versions.Marking these as required
AwareDatetimefields is likely correct for non‑runtime‑error results, but please verify no valid outputs omit them (e.g., edge cases on empty sources in older dbt versions). If any such cases exist, consider making them Optional.Option if needed:
- max_loaded_at: AwareDatetime - snapshotted_at: AwareDatetime + max_loaded_at: Optional[AwareDatetime] + snapshotted_at: Optional[AwareDatetime]
9-9: Good move — timezone-aware datetimes; ensure Pydantic v2 is pinnedScan of dependency files found no pydantic entry (only requirements.setup.txt was listed). Add an explicit Pydantic >=2.0 constraint (e.g. pydantic>=2.0,<3.0) in pyproject.toml / requirements.txt so AwareDatetime is available.
14-23: No change required — do not add invocation_started_at to sources v2 metadata. dbt 1.10 sources (freshness) v2 schema lists dbt_schema_version, dbt_version, generated_at, invocation_id, and env; it does not include metadata.invocation_started_at, so keep FreshnessMetadata unchanged.dbt_artifacts_parser/parsers/run_results/run_results_v5.py (2)
72-80: LGTM: Clear top-level artifact model.Nice encapsulation of the run_results payload with extra='forbid'. Fields match expected v5 shape.
9-9: Confirm Pydantic v2 usage (RootModel imported)dbt_artifacts_parser/parsers/run_results/run_results_v5.py imports RootModel (v2-only). Repo uses ConfigDict widely and the generator targets pydantic_v2; no @validator/BaseSettings (v1-only) usages were found. No explicit pydantic>=2 pin found in pyproject.toml / requirements* — add/verify pydantic>=2 in dependency files and CI.
| class RunResultsV5(RootModel[RunResultsArtifact]): | ||
| root: RunResultsArtifact |
There was a problem hiding this comment.
🧩 Analysis chain
RootModel switch is a breaking API change; add back-compat shims.
Consumers that previously did RunResultsV5(metadata=..., results=...) or accessed rr.results will break (now rr.root.results). Provide a before-model validator and proxy properties to preserve the old construction and attribute access.
Apply this diff to add back-compat:
-class RunResultsV5(RootModel[RunResultsArtifact]):
- root: RunResultsArtifact
+class RunResultsV5(RootModel[RunResultsArtifact]):
+ root: RunResultsArtifact
+
+ # Back-compat: allow constructing with flattened kwargs/dicts
+ @model_validator(mode='before')
+ @classmethod
+ def _coerce_root(cls, v):
+ if isinstance(v, dict) and 'root' not in v and any(
+ k in v for k in ('metadata', 'results', 'elapsed_time', 'args')
+ ):
+ return RunResultsArtifact(**v)
+ return v
+
+ # Back-compat: expose top-level fields as properties
+ @property
+ def metadata(self) -> BaseArtifactMetadata:
+ return self.root.metadata
+
+ @property
+ def results(self) -> List[RunResultOutput]:
+ return self.root.results
+
+ @property
+ def elapsed_time(self) -> float:
+ return self.root.elapsed_time
+
+ @property
+ def args(self) -> Optional[Dict[str, Any]]:
+ return self.root.argsAdditionally, import model_validator:
-from pydantic import ConfigDict, RootModel
+from pydantic import ConfigDict, RootModel, model_validatorVerification (grep for potential breakages):
🏁 Script executed:
#!/bin/bash
# Direct instantiation call sites that may pass flattened kwargs
rg -n -C2 '\bRunResultsV5\s*\(' --type py
# Usages expecting attribute access without .root
rg -n -C2 '\.results\b|\.metadata\b|\.elapsed_time\b|\.args\b' --type py | rg -n -C0 'RunResultsV5' -B3 -A0Length of output: 1239
RootModel switch is a breaking API change; add back-compat shims.
dbt_artifacts_parser/parser.py constructs RunResultsV5(**run_results) at dbt_artifacts_parser/parser.py:250 and :291 — this will break consumers and the parser. Apply the back-compat validator + proxy properties and import change shown below.
-from pydantic import ConfigDict, RootModel
+from pydantic import ConfigDict, RootModel, model_validator-class RunResultsV5(RootModel[RunResultsArtifact]):
- root: RunResultsArtifact
+class RunResultsV5(RootModel[RunResultsArtifact]):
+ root: RunResultsArtifact
+
+ # Back-compat: allow constructing with flattened kwargs/dicts
+ @model_validator(mode='before')
+ @classmethod
+ def _coerce_root(cls, v):
+ if isinstance(v, dict) and 'root' not in v and any(
+ k in v for k in ('metadata', 'results', 'elapsed_time', 'args')
+ ):
+ return RunResultsArtifact(**v)
+ return v
+
+ # Back-compat: expose top-level fields as properties
+ @property
+ def metadata(self) -> BaseArtifactMetadata:
+ return self.root.metadata
+
+ @property
+ def results(self) -> List[RunResultOutput]:
+ return self.root.results
+
+ @property
+ def elapsed_time(self) -> float:
+ return self.root.elapsed_time
+
+ @property
+ def args(self) -> Optional[Dict[str, Any]]:
+ return self.root.argsCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In dbt_artifacts_parser/parsers/run_results/run_results_v5.py around lines
82-83, the change to inherit RootModel and require a top-level "root" field
breaks existing callers that construct RunResultsV5(**run_results). Restore
backward compatibility by adding a pre root_validator that detects when input is
already the artifact dict and wraps it into {"root": input}, add proxy
properties (or __getattr__) on RunResultsV5 to forward attribute access to
self.root for the original top-level fields, and ensure any external imports
still reference this class (adjust import aliases if needed) so existing
parser.py calls at lines ~250 and ~291 continue to work without changes.
d24fec6 to
856281a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dbt_artifacts_parser/parsers/manifest/manifest_v12.py (1)
743-743: Nit: prefer enum member as default over raw stringUsing the enum member is a bit clearer and type-safe (mypy-friendly). If this file is code‑generated, consider adjusting the generator; otherwise:
- updates_on: Optional[UpdatesOn] = 'any' + updates_on: Optional[UpdatesOn] = UpdatesOn.anyAlso applies to: 2625-2625
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
dbt_artifacts_parser/resources/catalog/catalog_v1.jsonis excluded by!**/*.jsondbt_artifacts_parser/resources/manifest/manifest_v12.jsonis excluded by!**/*.jsondbt_artifacts_parser/resources/run-results/run-results_v6.jsonis excluded by!**/*.jsondbt_artifacts_parser/resources/sources/sources_v3.jsonis excluded by!**/*.json
📒 Files selected for processing (7)
dbt_artifacts_parser/__init__.py(1 hunks)dbt_artifacts_parser/parsers/catalog/catalog_v1.py(1 hunks)dbt_artifacts_parser/parsers/manifest/manifest_v12.py(12 hunks)dbt_artifacts_parser/parsers/run_results/run_results_v5.py(2 hunks)dbt_artifacts_parser/parsers/run_results/run_results_v6.py(2 hunks)dbt_artifacts_parser/parsers/sources/sources_v2.py(4 hunks)dbt_artifacts_parser/parsers/sources/sources_v3.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- dbt_artifacts_parser/init.py
- dbt_artifacts_parser/parsers/catalog/catalog_v1.py
- dbt_artifacts_parser/parsers/run_results/run_results_v5.py
- dbt_artifacts_parser/parsers/sources/sources_v3.py
- dbt_artifacts_parser/parsers/run_results/run_results_v6.py
🧰 Additional context used
🧬 Code graph analysis (1)
dbt_artifacts_parser/parsers/manifest/manifest_v12.py (1)
dbt_artifacts_parser/parsers/base.py (1)
BaseParserModel(23-26)
🔇 Additional comments (11)
dbt_artifacts_parser/parsers/sources/sources_v2.py (4)
20-20: LGTM switching to AwareDatetime; keep default string only if schema mandates it.If not mandated by the JSON schema, consider defaulting to None to avoid a fixed timestamp default.
64-65: LGTM: timing fields now timezone‑aware.This aligns with dbt’s ISO8601 “Z” timestamps.
9-9: Require Pydantic v2 for AwareDatetime/ConfigDict
AwareDatetime and ConfigDict are Pydantic v2-only — ensure project manifests and CI specify pydantic>=2.0 (check pyproject.toml / setup.cfg / requirements*.txt / poetry.lock and CI config).
82-83: Timezone-awareness enforced for required fields — fixtures OKScanned JSON artifacts for snapshotted_at / max_loaded_at / started_at / completed_at / generated_at — no timezone-less datetimes found; fixtures appear timezone-aware.
dbt_artifacts_parser/parsers/manifest/manifest_v12.py (7)
732-735: New UpdatesOn enum — LGTMMatches upstream values ('all' | 'any').
741-743: build_after made required — verify back-compat expectationsBuildAfter.count/period and Freshness.build_after are now required. If manifest_v12 artifacts from 1.9 could omit these, this is a breaking parse change. If v12 in dbt 1.10 guarantees presence, this is fine. Please confirm intended compatibility surface.
Also applies to: 750-750
1562-1566: Source config enrichment — LGTMAdding freshness, loaded_at_field, loaded_at_query, meta, tags to Config20 aligns with dbt 1.10 and PR goals.
1589-1589: Sources.freshness binding — LGTM; check consistency across disabled variantsThis switches to Freshness1 for Sources. Ensure Disabled8 uses the corresponding Freshness4 variant (it does below) so representations stay consistent.
2620-2625: Model freshness variants required — verify intentBuildAfter1/Freshness3 are also now required in model config variants; consistent with the earlier change. Confirm this mirrors the dbt 1.10 manifest v12 schema to avoid rejecting older artifacts inadvertently.
Also applies to: 2629-2633, 2668-2668
3313-3320: Approve: source freshness variants + loaded_at_ fields verified*Freshness4/Freshness5 and loaded_at_field/loaded_at_query are defined and propagated into Sources, Config44, and Disabled8.
30-33: Scope check + version default alignmentmanifest_v12 sets dbt_version = '1.10.8' while related parsers use '1.10.0a1' — confirm this bump is intentional and either align defaults across parsers or document the divergence.
- dbt_artifacts_parser/parsers/manifest/manifest_v12.py: dbt_version = '1.10.8'
- dbt_artifacts_parser/parsers/run_results/run_results_v6.py: dbt_version = '1.10.0a1'
- dbt_artifacts_parser/parsers/catalog/catalog_v1.py: dbt_version = '1.10.0a1'
- dbt_artifacts_parser/parsers/sources/sources_v3.py: dbt_version = '1.10.0a1'
dlouseiro
force-pushed
the
dlouseiro/add-support-for-dbt-1.10-in-run-results
branch
from
856281a to
65865c8
Compare
dlouseiro
changed the title
run_results, catalog and sourcesrun_results
dlouseiro
force-pushed
the
dlouseiro/add-support-for-dbt-1.10-in-run-results
branch
from
701349e to
4bb2e79
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dbt_artifacts_parser/resources/manifest/manifest_v12.jsonis excluded by!**/*.jsondbt_artifacts_parser/resources/run-results/run-results_v6.jsonis excluded by!**/*.json
📒 Files selected for processing (2)
dbt_artifacts_parser/parsers/manifest/manifest_v12.py(13 hunks)dbt_artifacts_parser/parsers/run_results/run_results_v6.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
dbt_artifacts_parser/parsers/manifest/manifest_v12.py (1)
dbt_artifacts_parser/parsers/base.py (1)
BaseParserModel(23-26)
🔇 Additional comments (1)
dbt_artifacts_parser/parsers/run_results/run_results_v6.py (1)
22-22: LGTM: added invocation_started_atField addition is correct and optional; compatible with v1.10.
|
@yu-iskw Hi! I'm also impacted by this issue, and confirm this PR fixes it. Any way to get it merged? Let me know if I can help. |
|
0.11.0 is out. |
3 participants
User description
The purpose of this PR is to ensure
run_results_v6is compatible with version 1.10 of dbt, resolving this issue.Even though the support for dbt 1.10 was added to the manifest here, it was not done for the
run_results.Implementation details:
dbt_artifacts_parser/resources/run-results/run-results_v6.jsonmetadata.invocation_started_at) and missing enum value for status (no-op) indbt_artifacts_parser/parsers/run_results/run_results_v6.pyPR Type
Enhancement
Description
Add dbt v1.10 compatibility to run_results_v6 parser
Include missing
invocation_started_atmetadata fieldAdd
no-opstatus enum valueUpdate version to 0.9.1
Changes diagram
Changes walkthrough 📝
__init__.py
Version bump to 0.9.1dbt_artifacts_parser/init.py
run_results_v6.py
Add dbt v1.10 fields and enumsdbt_artifacts_parser/parsers/run_results/run_results_v6.py
invocation_started_atfield to Metadata classno_openum value to Status classrun-results_v6.json
Update JSON schema for dbt v1.10dbt_artifacts_parser/resources/run-results/run-results_v6.json
invocation_started_atfield to metadata schemano-opvalue to status enum in schema