Skip to content

Model Deprecation #7562

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
peterallenwebb merged 12 commits into from
May 23, 2023
Merged

Model Deprecation #7562

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7cc1d74
CT-2461: Work toward model deprecation
peterallenwebb May 8, 2023
55debbe
CT-2461: Remove unneeded conversions
peterallenwebb May 9, 2023
9ca7d37
CT-2461: Fix up unit tests for new fields, correct a couple oversights
peterallenwebb May 9, 2023
fd88586
CT-2461: Remaining implementation and tests for model/ref deprecation…
peterallenwebb May 10, 2023
7842dca
CT-2461: Changelog entry for deprecation warnings
peterallenwebb May 10, 2023
37c8361
CT-2461: Refine datetime handling and tests
peterallenwebb May 10, 2023
ccd73b0
CT-2461: Fix up unit test data
peterallenwebb May 10, 2023
8277e8c
CT-2461: Fix some more unit test data.
peterallenwebb May 10, 2023
0637591
Merge remote-tracking branch 'origin' into paw/ct-2461-model-deprecation
peterallenwebb May 19, 2023
d9a4cc2
CT-2461: Fix merge issues
peterallenwebb May 19, 2023
ed443da
CT-2461: Code review items.
peterallenwebb May 22, 2023
e64efb1
CT-2461: Improve version -> str conversion
peterallenwebb May 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
CT-2461: Refine datetime handling and tests
  • Loading branch information
@peterallenwebb
peterallenwebb committed May 10, 2023
commit 37c8361e34d3f0d27804543ab0d99131a5edb606
19 changes: 19 additions & 0 deletions core/dbt/contracts/graph/unparsed.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,8 @@ def __post_init__(self):
else:
self._unparsed_columns.append(column)

self.deprecation_date = normalize_date(self.deprecation_date)


@dataclass
class UnparsedAnalysisUpdate(HasConfig, HasColumnDocs, HasColumnProps, HasYamlMetadata):
Expand Down Expand Up @@ -232,6 +234,8 @@ def __post_init__(self):

self._version_map = {version.v: version for version in self.versions}

self.deprecation_date = normalize_date(self.deprecation_date)

def get_columns_for_version(self, version: NodeVersion) -> List[UnparsedColumn]:
if version not in self._version_map:
raise DbtInternalError(
Expand Down Expand Up @@ -655,3 +659,18 @@ def validate(cls, data):
super(UnparsedGroup, cls).validate(data)
if data["owner"].get("name") is None and data["owner"].get("email") is None:
raise ValidationError("Group owner must have at least one of 'name' or 'email'.")


def normalize_date(d: Optional[datetime.date]) -> Optional[datetime.datetime]:
"""Convert date to datetime (at midnight), and add local time zone if naive"""
if d is None:
return None

# convert date to datetime
dt = d if type(d) == datetime.datetime else datetime.datetime(d.year, d.month, d.day)

if not dt.tzinfo:
Copy link
Contributor

@dbeatty10 dbeatty10 May 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Python docs here say the following:

image

Translating those into code gives something like this.

The slight change suggested below would cover the case where tzinfo is not None but dt.tzinfo.utcoffset(dt) returns None.

To fully encode the Python datetime definition, I think we'd need to do something like this:

Suggested change
if not dt.tzinfo:
if not dt.tzinfo or not dt.tzinfo.utcoffset(dt):

Granted, it might "never" happen for a tzinfo object from pytz to return None for the offset, but I was able to hand construct such a tzinfo object locally.

Copy link
Contributor Author

@peterallenwebb peterallenwebb May 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be happy to tighten up this check if you're willing to open the issue. Sorry we didn't get it in v1.

# date is naive, re-interpret as system time zone
dt = dt.astimezone()

return dt
13 changes: 8 additions & 5 deletions core/dbt/parser/manifest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,11 +539,14 @@ def load(self):
def check_for_model_deprecations(self):
for node in self.manifest.nodes.values():
if isinstance(node, ModelNode):
if node.deprecation_date and node.deprecation_date < datetime.datetime.now():
if (
node.deprecation_date
and node.deprecation_date < datetime.datetime.now().astimezone()
Copy link
Contributor

@dbeatty10 dbeatty10 May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peterallenwebb want to double-check two things with you:

  1. naive deprecation_dates
  2. single stable time

naive deprecation_dates

I believe this would be dependent on the system time wherever the python is being executed. So we'll probably want to double-check that any deprecation_date expressed without an offset like 2023-05-17 is understood to have its offset from the system time zone.

single stable time

Were you able to try out something akin to run_started_at?

It might be useful for using a single stable value over the course of the entire execution. It might also be useful from a testing perspective as well.

Copy link
Contributor Author

@peterallenwebb peterallenwebb May 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbeatty10 Yes, your understanding is correct, and the function you commented on elsewhere ensures that any date or datetime without an explicit timezone is re-interpreted as being in the system timezone and made non-naive.

I wasn't able to figure out a convenient way of accessing run_started_at in the context where this is evaluated, but it is worth following up on when we have some time, IMO. I agree with your point about testing, especially, which often overlooked even by experienced engineers.

):
fire_event(
DeprecatedModel(
model_name=node.name,
model_version=node.version,
model_version=str(node.version),
deprecation_date=node.deprecation_date.isoformat(),
)
)
Expand All @@ -553,7 +556,7 @@ def check_for_model_deprecations(self):
for resolved_ref in resolved_model_refs:
if resolved_ref.deprecation_date:

if resolved_ref.deprecation_date < datetime.datetime.now():
if resolved_ref.deprecation_date < datetime.datetime.now().astimezone():
event_cls = DeprecatedReference
else:
event_cls = UpcomingReferenceDeprecation
Expand All @@ -563,8 +566,8 @@ def check_for_model_deprecations(self):
model_name=node.name,
ref_model_package=resolved_ref.package_name,
ref_model_name=resolved_ref.name,
ref_model_version=resolved_ref.version,
ref_model_latest_version=resolved_ref.latest_version,
ref_model_version=str(resolved_ref.version),
ref_model_latest_version=str(resolved_ref.latest_version),
Copy link
Contributor

@MichelleArk MichelleArk May 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto here and on the line above re: string conversion and error messages: https://github.com/dbt-labs/dbt-core/pull/7562/files#r1199287517

ref_model_deprecation_date=resolved_ref.deprecation_date.isoformat(),
)
)
Expand Down
2 changes: 1 addition & 1 deletion tests/functional/artifacts/expected_manifest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1514,7 +1514,7 @@ def expected_versions_manifest(project):
"depends_on": {"macros": [], "nodes": [], "public_nodes": []},
"deferred": False,
"description": "A versioned model",
"deprecation_date": None,
"deprecation_date": ANY,
"docs": {"node_color": None, "show": True},
"fqn": ["test", "versioned_model", "v1"],
"group": "test_group",
Expand Down