[4/n][dagster-sling] Mark DagsterSlingTranslator.get_* methods as superseded

[maximearmstrong]

Summary & Motivation

As title.

[maximearmstrong]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• [6/n][dagster-sling] Update docs with DagsterSlingTranslator.get_asset_spec #27168
• [5/n][dagster-sling] Update callsites with DagsterSlingTranslator.get_asset_spec #27167
• [4/n][dagster-sling] Mark DagsterSlingTranslator.get_* methods as superseded #27142 👈 (View in Graphite)
• [3/n][dagster-sling] Leverage get_asset_spec in DagsterSlingTranslator.get_* #27141
• [2/n][dagster-sling] Move logic to default fns in DagsterSlingTranslator #27140
• [1/n][dagster-sling] Move asset spec creation to DagsterSlingTranslator.get_asset_spec #27139
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

[cmpadden]

@maximearmstrong , I'm curious if it's necessary to deprecate the get_ methods, with get_asset_spec being the favored approach. Is this being standardized across all integrations, dbt, dlt, etc?

I just want to be cognizant of the impact it has on our users who having working pipelines, and want them to continue to work with little to no changes. I could imagine us having warnings on their usage, and updating all learning material to prefer get_asset_spec but avoid deprecation.

[OwenKephart]

I agree with @cmpadden 's points here -- I think maybe superseded is a better decorator here? At the very least the deprecation cycle should be longer

I think the maintenance burden of these extra methods isn't too high on our side (even if they're a bit unsightly)

[OwenKephart]

Following up now that I've looked at the downstack PR, I think there's a way to delete these methods without breaking users and without having to keep these in code forever

Also having thought about it more, I'm not sure there's a good way for us to actually mark these as deprecated even if we wanted to -- the user is overriding these functions not calling them, so marking them this way won't really alert the user that anything is "wrong"

[maximearmstrong]

Also having thought about it more, I'm not sure there's a good way for us to actually mark these as deprecated even if we wanted to -- the user is overriding these functions not calling them, so marking them this way won't really alert the user that anything is "wrong"

@OwenKephart I updated the decorator to mark methods as superseded. I think keeping the decorators and methods are important:

• This will ensure that get_* methods are marked as superseded in the docs.
• These methods can be manually called by users.
  • Nothing prevents users to do something like DagsterSlingTranslator().get_asset_key(stream_definition) in their code to recreate an asset key - it's not best practice, but not forbidden.

To make sure users get the supersession warning, I updated _resolve_back_compat_method to emit a warning if a get_* method is used.

q

[maximearmstrong]

I'm curious if it's necessary to deprecate the get_ methods, with get_asset_spec being the favored approach. Is this being standardized across all integrations, dbt, dlt, etc?

@cmpadden This thread can give you more context on why we think get_ methods should be deprecated.

The main problem is that users can override get_asset_key without using the correct and new implementation in get_asset_spec. This caused many problems in the past, where users would have asset key with different prefixes, or asset duplication, because of that.

The conclusion was that only get_asset_spec should be implemented, and then attributes could be accessed from the AssetSpec object.

[OwenKephart]

Note my comment on the warning message

[OwenKephart]

so this warning comes up specifically in cases where users are overriding the method on the base class, so maybe the warning should reference that? i.e. Instead of overriding X, override DagsterSlingTranslator.get_asset_spec()