Model Deprecation

[peterallenwebb]

resolves #7433

Description

Support model deprecation, with appropriate warnings and errors.

Checklist

• I have read the contributing guide and understand what's expected of me
• I have signed the CLA
• I have run this code in development and it appears to resolve the stated issue
• This PR includes tests, or tests are not required/relevant for this PR
• I have opened an issue to add/update docs, or docs changes are not required/relevant for this PR
  • deprecation_date for model versions docs.getdbt.com#3357
• I have run changie new to create a changelog entry

[github-actions]

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

[gshank]

Wow. This was a lot more complicated than anticipated, but does seem like functionality that we want to have.

At one point the server code was also creating a msgpack'd manifest, so we might want to check their codebase to see if they need to use this too.

[MichelleArk]

The msgpack docs outline an alternative method to encoding custom types by returning something more like:

{'__datetime__': True, 'as_str': obj.strftime("%Y%m%dT%H:%M:%S.%f")}

It looks to be interchangeable with ExtType usage as you've implemented. The ExtType approach feels more readable + extensible to custom data types beyond datetime

[dbeatty10]

@MichelleArk For the implementation, would all models have their deprecation_dates compared against a shared datetime.now(pytz.utc)? Or would they each get their own datetime.now(pytz.utc) depending on when their deprecation_date is evaluated?

When the dbt Developer Experience (DX) team was first reviewing the design for deprecated_date as part of dbt model versions, @joellabes raised the idea of using run_started_at as a consistent datetime to use across all models in a single dbt execution.

There could be other ways of generating a shared datetime.now(pytz.utc) too besides run_started_at, but it seemed like an easy one to reach for.

[peterallenwebb]

@dbeatty10 I like the idea of comparing against run_started_at. I'll try that.

[peterallenwebb]

@dbeatty10 Do you think we need a documentation case for this work?

[dbeatty10]

@dbeatty10 Do you think we need a documentation case for this work?

@peterallenwebb Yes, indeed -- thanks for noticing and asking!

Just created the relevant placeholder issue for the related documentation here and checked it off in the checklist:

[dbeatty10]

@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.

[peterallenwebb]

@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.

[gshank]

Looks good, just need to merge main.

[MichelleArk]

✨

[MichelleArk]

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

[dbeatty10]

Left a few comments on some edge cases.

[dbeatty10]

The Python docs here say the following:

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.

[peterallenwebb]

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.

[dbeatty10]

Are deprecation_date formats like the following accepted?

• 2022-04-08T22:00:00.0-04:00
• 2022-04-08 22:00:00.0-04:00
• 2022-04-08T22:00:00.0
• 2022-04-08 22:00:00.0

If so, could we add test cases for the relevant ones?

@MichelleArk Could you or someone else add which cases do or don't work in the docs issue here?

[peterallenwebb]

@dbeatty10 We are using PyYAML to parse JSON, and relying on its date detection and parsing for now, which claims to follow the YAML spec. I'll figure out what we can say definitively about the dates we accept.