[dagster-dbt] update usage of deprecated context methods #19806
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Current dependencies on/for this PR: This stack of pull requests is managed by Graphite. |
rexledesma
reviewed
Feb 14, 2024
Comment on lines
971
to
980
| op_context = ( | ||
| context.op_execution_context if isinstance(context, AssetExecutionContext) else context | ||
| ) |
There was a problem hiding this comment.
Would prefer that we just keep this named context.
Can we move this to be after we instantiate assets_def, but before the if context and assets_def is not None?
Suggested change
| op_context = ( | |
| context.op_execution_context if isinstance(context, AssetExecutionContext) else context | |
| ) | |
| context: Optional[OpExecutionContext] = ( | |
| context.op_execution_context if isinstance(context, AssetExecutionContext) else context | |
| ) |
There was a problem hiding this comment.
sure for the rename, but _get_unique_target_name needs the op context too and is called before we get the assets_def. I just moved fetching assets_def up, but lmk if you'd prefer something else
932e85e
to
00d90d5
Compare
rexledesma
approved these changes
Feb 15, 2024
python_modules/libraries/dagster-dbt/dagster_dbt/core/resources_v2.py
Outdated
Show resolved
Hide resolved
00d90d5
to
bc6f559
Compare
PedramNavid
pushed a commit
that referenced
this pull request
Mar 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary & Motivation
Some methods in the dbt integration use context methods that were deprecated in #19441 and #17339. The op-specific methods are meant to be replaced by calling
context.op_execution_context.oprather thancontext.op. However, in some of the dbt methods, we don't know if the context is of typeAssetExecutionContextorOpExecutionContext, so it is unclear if we need to call the underlyingop_execution_contextbefore op-specific methods.This PR adds a check in some methods that will pull the
op_execution_contextfrom the context if the context is anAssetExecutionContextso that we are guaranteed to be working with anOpExecutionContextin the rest of the method.How I Tested These Changes