Only have one kind of context for direct invocation #17554

jamiedemaria · 2023-10-31T21:31:03Z

Summary & Motivation

This PR merges UnboundOpExecutionContext and BoundOpExecutionContext into a single object, DirectOpExecutionContext.

There are three states the DirectOpExecutionContext can be in:

pre-execution. The context is not tied to a particular asset or op to be executed. Therefore information like op_config or asset_key is not accessible. We call this state "unbound" or "not bound"
during execution. The context is now tied to a particular asset or op that is being executed. The information above (like op_config) is now accessible. Additionally, the user may do things that mutate the context, like emit user events or add output metadata. We call this state "bound". This state of the context is the BoundOpExecutionContext in the old version of the code.
post-execution. The context is no longer tied to a particular op/asset execution, however, information about emitted events or output metadata still needs to be available to the user so they can assert on the contents of these events. The context is also considered "unbound" in this state.

In order to make the distinction between these states more clear and easier to maintain for engineers, the attributes associated with each state are split into separate objects:
BoundProperties - maintains properties that are only available when the context is bound to an asset or op execution. When the context is bound to an invocation of a particular op, the BoundProperties object will be instantiated with the relevant properties for the context as bound to that op. At the end of execution, this object is deleted. We track the bound/unbound state of the context by the existence of the BoundProperties object.
DirectExecutionProperties - maintains attributes that can only be mutated while the context is bound, but can be read at any time (pre or post execution). If another execution begins using the same context, the DirectExecutionProperties object is replaced with a fresh instantiation.

Previous PR that does a similar thing but wasn't merged #15083

This PR accomplishes two main goals:

Makes is easier to add an AssetExecutionContext for direct invocation since the pattern for a direct invocation context is simplified.
Generally cleans up the direct invocation path by removing a confusing section of code. Sean put it well:
- Motivation:
  Currently there are two classes UnboundOpExecutionContext and BoundOpExecutionContext. Both classes inherit from OpExecutionContext. UnboundOpExecutionContext is returned by build_op_context, i.e. it is used for direct op invocation. This is converted to a BoundOpExecutionContext in the call method of OpDefinition when an op is invoked.
  ~ The status quo is problematic for a few reasons:
  ~ The names (Un)BoundOpExecutionContext are confusing. It makes it sound like these classes are used for all op executions, when in fact they are never used as part of a run-- only for what is called "invocation".
  There is a large amount of code duplication between the two classes. This is hard to maintain and probably a source of bugs.

How I Tested These Changes

new unit tests

jamiedemaria · 2023-10-31T21:31:14Z

Current dependencies on/for this PR:

master
- PR Only have one kind of context for direct invocation #17554 👈
  - PR duplicate op context methods on asset context #18573
    - PR deprecate methods on AssetExecutionContext that can be found via the DagsterRun object #17339
      - PR Direct invocation asset execution context #18549
        
        PR deprecate asset_partition_*_for_output on AssetExecutionContext #19436
        
        PR [docs] remove asset_partition_*_for_output from docs #19437
        
        PR deprecate op related methods from AssetExecutionContext #19441
        
        PR Make upstream asset materialization events available on the context #18971
        
        PR [wip] store the tags fetched by data versioning so we can use them for upath io manager #19324
        
        PR [UPath I/O managers] special case handling of None outputs #18820
        
        PR Make input partitions methods based on asset key not input name #19027
        
        PR Support partitioned assets for fetching latest AssetMaterializations #19286
        
        PR [do not merge] base example to showcase new apis #19199
        
        PR [prototype] sub-context for each upstream dependency #19283
        
        PR [prototype] AssetExecutionContext partition methods on sub context #19236
        
        PR [prototype] AssetExecutionContext partition methods given as named tuple #19037
        
        PR [prototype] AssetExecutionContext partition methods directly on the context #19032
        
        PR [wip] two execution contexts with minimal changes #18543
        
        PR [wip] see wht needs to change when asset context is no longer an op context #18553
      - PR support Asset and Op ExecutionContexts in standard execution path #17972
        
        PR [wip] add AssetExecutionContext to direct invocation #18044

This stack of pull requests is managed by Graphite.

jamiedemaria · 2023-11-01T19:36:12Z

python_modules/dagster/dagster/_core/execution/context/invocation.py

    )


-class UnboundOpExecutionContext(OpExecutionContext):
-    """The ``context`` object available as the first argument to a solid's compute function when
+class DirectInvocationOpExecutionContext(OpExecutionContext):


i think this is fine, much clearer than previous name

Would like to suggest RunlessOpExecutionContext:

Aligns with "runless" events

Communicates more about what makes this different from a regular OpExecutionContext-- contextual info about the run is missing

"Invocation" is not very self-explanatory and is overloaded-- it is used for instance when building a graph with the composition API inside @job (PendingNodeInvocation)

Shorter than DirectInvocationOpExecutionContext

^ I find this thinking compelling. Would also be ok with just DirectOpExecutionContext

Also relevant: #18265 (comment)

but it should be since it is returned by public functions that build test contexts.

I think of it as an implementation detail "private" subclass of the real public parent class. I believe the typehints on those build test context methods are the public parent classes. It doesn't quite live up to the aspirations of being able to treat it like an OpExecutionContext and have everything work. I do like treating it as private and being able to change it.

^^ ok makes sense

@schrockn do you like TestOpExecutionContext?

bump for @schrockn to weigh in on naming

Aligning on DirectOpExecutionContext. Reasoning:

Some users do full integration tests (like calling materialize) in these cases they won't be getting a "test" context, but they are still in a testing setup.

Direct side steps this issue because it's for the case when you are "directly" providing a context object to an op/asset

If we realize later the name needs to be different, the impact of changing the name later is low since it's a non-exported class

jamiedemaria · 2023-11-01T19:37:00Z

python_modules/dagster/dagster/_core/execution/context/invocation.py

-    been validated.
-    """
-
-    _op_def: OpDefinition


I don't really understand what these were in the BoundOpExecutionContext for. I can add them to the new class though if they are needed!

For historical context here - op_def was added so that after you've actually invoked an op, you could access definition-level properties about that op on the context object itself. This is useful for the situation where you use op factories for example, and might still need to query definition-level information.

I thought since these were on the BoundExecutionContext that they wouldn't be accessible after invocation since the bound context gets garbage collected

right - i meant during invocation.

ah ok - this should still work in the new setup

github-actions · 2023-11-02T19:17:09Z

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-eesafdcx5-elementl.vercel.app
https://jamie-level-1-collapse-DI-contexts.components-storybook.dagster-docs.io

Built with commit 3096474.
This pull request is being automatically deployed with vercel-action

github-actions · 2023-11-02T19:17:27Z

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-inlpsqbme-elementl.vercel.app
https://jamie-level-1-collapse-DI-contexts.core-storybook.dagster-docs.io

Built with commit 3096474.
This pull request is being automatically deployed with vercel-action

github-actions · 2023-11-02T19:20:58Z

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-px1j6c7ou-elementl.vercel.app
https://jamie-level-1-collapse-DI-contexts.dagster.dagster-docs.io

Direct link to changed pages:

...dagster/dagster_tests/core_tests/resource_tests/pythonic_resources/test_direct_invocation.py

github-actions · 2023-11-16T21:12:02Z

Deploy preview for dagster-university ready!

✅ Preview
https://dagster-university-gclhjjgau-elementl.vercel.app
https://jamie-level-1-collapse-DI-contexts.dagster-university.dagster-docs.io

Built with commit 3096474.
This pull request is being automatically deployed with vercel-action

jamiedemaria commented Nov 1, 2023

View reviewed changes

jamiedemaria force-pushed the jamie/level-1-collapse-DI-contexts branch from 05af4f3 to 1387482 Compare November 2, 2023 19:13

jamiedemaria mentioned this pull request Nov 6, 2023

add some direct invocation test cases #17719

Merged

jamiedemaria force-pushed the jamie/level-1-collapse-DI-contexts branch 2 times, most recently from 1116a08 to 8e5aa5a Compare November 13, 2023 18:30

jamiedemaria commented Nov 13, 2023

View reviewed changes

...dagster/dagster_tests/core_tests/resource_tests/pythonic_resources/test_direct_invocation.py Outdated Show resolved Hide resolved

jamiedemaria changed the title ~~[wip] [prototype] Only have one kind of context for direct invocation~~ [RFC] Only have one kind of context for direct invocation Nov 13, 2023

jamiedemaria marked this pull request as ready for review November 13, 2023 18:34

jamiedemaria requested review from alangenfeld, smackesey and dpeng817 November 13, 2023 18:35

This was referenced Nov 13, 2023

deprecate methods on AssetExecutionContext that can be found via the DagsterRun object #17339

Merged

support Asset and Op ExecutionContexts in standard execution path #17972

Closed

jamiedemaria force-pushed the jamie/level-1-collapse-DI-contexts branch 2 times, most recently from 3df3b58 to 4de29b1 Compare November 15, 2023 17:13

jamiedemaria mentioned this pull request Nov 15, 2023

[wip] add AssetExecutionContext to direct invocation #18044

Closed

jamiedemaria force-pushed the jamie/level-1-collapse-DI-contexts branch 2 times, most recently from f8b02ae to 96e242d Compare November 16, 2023 21:09

jamiedemaria changed the base branch from master to jamie/update-build-asset-context-usages November 17, 2023 21:46

jamiedemaria force-pushed the jamie/level-1-collapse-DI-contexts branch from 96e242d to dee10a0 Compare November 17, 2023 21:46

This was referenced Nov 17, 2023

update build_op_context to build_asset_context where relevant #16673

Merged

support direct invocation with AssetExecutionContext #16635

Closed

test AssetExecutionContext subclass deprecations #16598

Closed

jamiedemaria force-pushed the jamie/level-1-collapse-DI-contexts branch from dee10a0 to a31a3da Compare November 17, 2023 21:53

jamiedemaria added 25 commits January 29, 2024 10:38

update boundproperties to be a plain class so attrs are mutable

e2827d5

wip

7fa3db6

add tests for different execution types

19ad255

fix dictionary check

aad8d50

test fixes

b23b982

test update

1a3aba4

test demo for unbinding on errors

a3b273d

re-org tests

08a9549

update comments

9273e00

handle raised errors

858c510

make pyright happy

d737f5f

re-org to invocation props

eff0b6a

clean up tests

b725535

rename DirectInvocationOpExecutionContext to RunlessOpExecutionContext

7edf1b2

rename to runlessexecutionproperties

1548074

use bound properties in invocation

fc1c282

make things properties

051d742

fix new fn callsite

1011311

fix test

9fa1d0e

access via property

129bfee

add is_bound prop

9537bde

use a methods that's actually on the context

0371e8e

rename

deec5a3

comments

46328a5

missed a name

92ea8d8

jamiedemaria force-pushed the jamie/level-1-collapse-DI-contexts branch from 3096474 to 92ea8d8 Compare January 29, 2024 15:38

jamiedemaria added 2 commits January 29, 2024 11:29

final cleanup

5f174fe

update boundproperties to perinvocationproperties

8e4327d

jamiedemaria merged commit 07923e8 into master Jan 29, 2024
1 check passed

jamiedemaria deleted the jamie/level-1-collapse-DI-contexts branch January 29, 2024 18:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only have one kind of context for direct invocation #17554

Only have one kind of context for direct invocation #17554

jamiedemaria commented Oct 31, 2023 •

edited

Loading

jamiedemaria commented Oct 31, 2023 •

edited

Loading

jamiedemaria Nov 1, 2023

alangenfeld Nov 27, 2023

smackesey Nov 30, 2023 •

edited

Loading

alangenfeld Dec 4, 2023

smackesey Dec 4, 2023

alangenfeld Dec 7, 2023

smackesey Dec 7, 2023

smackesey Dec 7, 2023

jamiedemaria Dec 14, 2023

jamiedemaria Dec 19, 2023

jamiedemaria Nov 1, 2023

dpeng817 Dec 5, 2023

jamiedemaria Dec 5, 2023

dpeng817 Dec 27, 2023

jamiedemaria Dec 27, 2023

github-actions bot commented Nov 2, 2023 •

edited

Loading

github-actions bot commented Nov 2, 2023 •

edited

Loading

github-actions bot commented Nov 2, 2023 •

edited

Loading

github-actions bot commented Nov 16, 2023 •

edited

Loading

Only have one kind of context for direct invocation #17554

Only have one kind of context for direct invocation #17554

Conversation

jamiedemaria commented Oct 31, 2023 • edited Loading

Summary & Motivation

How I Tested These Changes

jamiedemaria commented Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smackesey Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 2, 2023 • edited Loading

github-actions bot commented Nov 2, 2023 • edited Loading

github-actions bot commented Nov 2, 2023 • edited Loading

github-actions bot commented Nov 16, 2023 • edited Loading

jamiedemaria commented Oct 31, 2023 •

edited

Loading

jamiedemaria commented Oct 31, 2023 •

edited

Loading

smackesey Nov 30, 2023 •

edited

Loading

github-actions bot commented Nov 2, 2023 •

edited

Loading

github-actions bot commented Nov 2, 2023 •

edited

Loading

github-actions bot commented Nov 2, 2023 •

edited

Loading

github-actions bot commented Nov 16, 2023 •

edited

Loading