test AssetExecutionContext subclass deprecations #16598

jamiedemaria · 2023-09-18T18:15:48Z

Summary & Motivation

Adds a test to ensure that AssetExecutionContext stays in sync with OpExecutionContext. We don't want methods added to OpExecutionContext without their corresponding deprecated versions added to AssetExecutionContext, otherwise splitting AssetExecutionContext into it's own class will be a breaking change. This test goes through each attr on OpExecutionContext and ensure that it's either:

a method we want to keep on AssetExecutionContext
has a deprecation warning on AssetExecutionContext
some other attribute we want to ignore (ie __ methods)

How I Tested These Changes

jamiedemaria · 2023-09-18T18:15:57Z

Current dependencies on/for this PR:

master
- PR update build_op_context to build_asset_context where relevant #16673
  - PR Only have one kind of context for direct invocation #17554
    - PR deprecate methods on AssetExecutionContext that can be found via the DagsterRun object #17339
      - PR support Asset and Op ExecutionContexts in standard execution path #17972
        
        PR [wip] add AssetExecutionContext to direct invocation #18044
  - PR support direct invocation with AssetExecutionContext #16635
    - PR test AssetExecutionContext subclass deprecations #16598 👈

This stack of pull requests is managed by Graphite.

jamiedemaria · 2023-09-18T18:40:19Z

python_modules/dagster/dagster_tests/core_tests/execution_tests/test_asset_execution_context.py

+
+    other_ignores = [
+        "_abc_impl",
+        # TODO - what to do about instance vars for OpExecutionContext? listed below


The below attributes are instance variables on OpExecutionContext that are not available on AssetExecutionContext because AssetExecutionContext overrides the __init__ of OpExecutionContext. We could:

make properties for each of these attrs on AssetExecutionContext and throw deprecation warnings

do nothing since the underscore implies they are private. But there's probably a user somewhere who is calling these, and their code would break

IMO we should be able to break users that are accessing private members

schrockn

Don't think we should intercept calls to private members...

schrockn · 2023-09-18T19:54:09Z

python_modules/dagster/dagster_tests/core_tests/execution_tests/test_asset_execution_context.py

+    # Test that every method on OpExecutionContext is either reimplemented by AssetExecutionContext
+    # or throws a deprecation warning on AssetExecutionContext
+
+    # If this test fails, it is likely because you added a method to OpExecutionContext without
+    # also adding that method to AssetExecutionContext. Please add the same method to AssetExecutionContext
+    # with an appropriate deprecation warning (see existing deprecated methods for examples).
+    # If the method should not be deprecated for AssetExecutionContext, please still add the same method
+    # to AssetExecutionContext and add the method name to asset_execution_context_attrs


great summary

jamiedemaria · 2023-10-13T19:16:49Z

converting to draft to get it out of review queues - this will need to be re-opened once we start deprecating methods on AssetExecutionContext

jamiedemaria · 2023-11-17T21:31:39Z

#17339 replaces this

jamiedemaria mentioned this pull request Sep 18, 2023

[quick] use correct context type annotations in tests #16507

Merged

jamiedemaria commented Sep 18, 2023

View reviewed changes

jamiedemaria marked this pull request as ready for review September 18, 2023 18:40

jamiedemaria requested review from alangenfeld, smackesey and schrockn September 18, 2023 18:40

schrockn requested changes Sep 18, 2023

View reviewed changes

jamiedemaria force-pushed the jamie/provide-asset-context branch from 12894b6 to ec70892 Compare September 18, 2023 20:14

jamiedemaria force-pushed the jamie/test-asset-context branch from 896a7d2 to 9d026e9 Compare September 18, 2023 20:14

jamiedemaria requested a review from yuhan September 18, 2023 20:53

jamiedemaria force-pushed the jamie/provide-asset-context branch from ec70892 to a09fa99 Compare September 19, 2023 14:27

jamiedemaria force-pushed the jamie/test-asset-context branch from 9d026e9 to beae87d Compare September 19, 2023 14:27

jamiedemaria mentioned this pull request Sep 19, 2023

partition methods on AssetExecutionContext #16625

Closed

2 tasks

jamiedemaria force-pushed the jamie/provide-asset-context branch from a09fa99 to bcf4e6e Compare September 19, 2023 21:38

jamiedemaria changed the base branch from jamie/provide-asset-context to jamie/direct-invocation September 19, 2023 21:39

jamiedemaria force-pushed the jamie/test-asset-context branch from beae87d to 945482e Compare September 19, 2023 21:39

jamiedemaria mentioned this pull request Sep 19, 2023

support direct invocation with AssetExecutionContext #16635

Closed

jamiedemaria force-pushed the jamie/direct-invocation branch from 682fb19 to d596c86 Compare September 19, 2023 23:43

jamiedemaria force-pushed the jamie/test-asset-context branch from 945482e to 9fbf870 Compare September 19, 2023 23:43

jamiedemaria force-pushed the jamie/direct-invocation branch from d596c86 to 9c78c7e Compare September 20, 2023 00:04

jamiedemaria force-pushed the jamie/test-asset-context branch from 9fbf870 to 5d3c659 Compare September 20, 2023 00:04

jamiedemaria force-pushed the jamie/direct-invocation branch from 9c78c7e to 7c6b42a Compare September 20, 2023 16:43

jamiedemaria force-pushed the jamie/test-asset-context branch from 5d3c659 to c9751d3 Compare September 20, 2023 16:43

jamiedemaria force-pushed the jamie/direct-invocation branch from 7c6b42a to 431ffab Compare September 20, 2023 17:08

jamiedemaria force-pushed the jamie/test-asset-context branch from ef91ce0 to 72e04ec Compare September 22, 2023 13:08

jamiedemaria force-pushed the jamie/direct-invocation branch from 1919f73 to 384d390 Compare September 22, 2023 14:27

jamiedemaria force-pushed the jamie/test-asset-context branch from 72e04ec to 3d8e088 Compare September 22, 2023 14:27

jamiedemaria force-pushed the jamie/direct-invocation branch from 384d390 to 5c4494f Compare September 22, 2023 15:16

jamiedemaria force-pushed the jamie/test-asset-context branch from 3d8e088 to 5fad4f7 Compare September 22, 2023 15:16

jamiedemaria force-pushed the jamie/direct-invocation branch from 5c4494f to 2c59e3b Compare September 22, 2023 16:02

jamiedemaria force-pushed the jamie/test-asset-context branch from 5fad4f7 to 4c8533d Compare September 22, 2023 16:02

jamiedemaria force-pushed the jamie/direct-invocation branch from 2c59e3b to ab3472a Compare September 22, 2023 17:47

jamiedemaria force-pushed the jamie/test-asset-context branch from 4c8533d to f821ec9 Compare September 22, 2023 17:47

jamiedemaria force-pushed the jamie/direct-invocation branch from ab3472a to 2b23f4f Compare September 22, 2023 20:50

jamiedemaria force-pushed the jamie/test-asset-context branch from f821ec9 to ff9ba61 Compare September 22, 2023 20:50

jamiedemaria added 8 commits September 26, 2023 10:05

start test

f9c68f1

test context

73bb8c8

ruff directives

00650a3

test asset context instance check

dddc425

update deprecation warning test

e16b12f

remove log event from ignores

d36c6a6

update test for interface changes

b4a47f1

remove methods from test

ee3aac2

jamiedemaria force-pushed the jamie/direct-invocation branch from 2b23f4f to db73b3c Compare September 26, 2023 14:06

jamiedemaria requested a review from erinkcochran87 as a code owner September 26, 2023 14:06

jamiedemaria force-pushed the jamie/test-asset-context branch from ff9ba61 to ee3aac2 Compare September 26, 2023 14:06

jamiedemaria marked this pull request as draft October 13, 2023 19:16

jamiedemaria closed this Nov 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test AssetExecutionContext subclass deprecations #16598

test AssetExecutionContext subclass deprecations #16598

jamiedemaria commented Sep 18, 2023 •

edited

Loading

jamiedemaria commented Sep 18, 2023 •

edited

Loading

jamiedemaria Sep 18, 2023

schrockn Sep 18, 2023

schrockn left a comment

schrockn Sep 18, 2023

jamiedemaria commented Oct 13, 2023

jamiedemaria commented Nov 17, 2023

test AssetExecutionContext subclass deprecations #16598

test AssetExecutionContext subclass deprecations #16598

Conversation

jamiedemaria commented Sep 18, 2023 • edited Loading

Summary & Motivation

How I Tested These Changes

jamiedemaria commented Sep 18, 2023 • edited Loading

jamiedemaria Sep 18, 2023

Choose a reason for hiding this comment

schrockn Sep 18, 2023

Choose a reason for hiding this comment

schrockn left a comment

Choose a reason for hiding this comment

schrockn Sep 18, 2023

Choose a reason for hiding this comment

jamiedemaria commented Oct 13, 2023

jamiedemaria commented Nov 17, 2023

jamiedemaria commented Sep 18, 2023 •

edited

Loading

jamiedemaria commented Sep 18, 2023 •

edited

Loading