Page MenuHomePhabricator
Differential D1494

Only initialize required resources during step execution
ClosedPublic
Actions

Authored by prha on Nov 26 2019, 10:23 PM.

Details

Reviewers
alangenfeld
max
nate
schrockn
Commits
R1:7292dd6be326: Only initialize required resources during step execution
Summary

Uses required_resource_keys to provide Resources objects in the context with *only* those keys. If the user does not specify required_resource_keys in a solid, no resources are provided to the context. This allows us to be more selective about what resources are spun up for different pipeline subset executions.

For example, a pipeline with an expensive resource (such as a Spark context) will spin up the context even if the current pipeline or execution plan subset does not require it. With this change, solids are forced to declare their resource dependencies and therefore only the resources required by the solids about to be executed will be booted up.

Custom types and plugins within types that required resources are not covered in this diff and will be addressed in a following diff.

Resolves https://github.com/dagster-io/dagster/issues/1937

Test Plan

BK

Diff Detail

Repository
R1 dagster
Branch
scope-resource-creation
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
schrockn updated this revision to Diff 6960.Nov 27 2019, 9:57 PM

up

Harbormaster failed remote builds in B5616: Diff 6960!Nov 27 2019, 10:05 PM
schrockn updated this revision to Diff 6961.Nov 27 2019, 10:12 PM

up

Harbormaster failed remote builds in B5613: Diff 6956!Nov 27 2019, 10:12 PM
Harbormaster failed remote builds in B5615: Diff 6958!
Harbormaster failed remote builds in B5614: Diff 6957!
Harbormaster failed remote builds in B5617: Diff 6961!Nov 27 2019, 10:19 PM
schrockn updated this revision to Diff 6963.Nov 27 2019, 10:40 PM

up

Harbormaster failed remote builds in B5619: Diff 6963!Nov 27 2019, 10:47 PM
schrockn updated this revision to Diff 6972.Nov 27 2019, 11:39 PM
schrockn edited the summary of this revision. (Show Details)
schrockn added a parent revision: D1501: Eliminate selector argument from PipelineDefinition.

upmessage

schrockn updated this revision to Diff 6975.Nov 27 2019, 11:43 PM

up

Harbormaster failed remote builds in B5625: Diff 6972!Nov 27 2019, 11:46 PM
Harbormaster completed remote builds in B5628: Diff 6975.Nov 27 2019, 11:51 PM
schrockn mentioned this in D1505: Check-in expected failure repros for #1950 and #1949.Nov 28 2019, 12:28 AM
schrockn updated this revision to Diff 6984.Nov 28 2019, 12:47 AM
schrockn retitled this revision from Only boot up required resources to Add strict mod to pipelines and only initialized required resources.
schrockn edited the summary of this revision. (Show Details)
schrockn added reviewers: alangenfeld, max, nate.

upmessage

schrockn updated this revision to Diff 6986.Nov 28 2019, 12:52 AM
schrockn edited the summary of this revision. (Show Details)

upmessage

Harbormaster completed remote builds in B5634: Diff 6984.Nov 28 2019, 12:55 AM
Harbormaster completed remote builds in B5636: Diff 6986.Nov 28 2019, 12:59 AM
nate accepted this revision.Dec 1 2019, 4:46 PM
nate added inline comments.
python_modules/dagster/dagster/core/definitions/system_storage.py
81

I doubt anyone is using system_storage in the wild, but note this is a small breaking external API change

python_modules/dagster/dagster/core/execution/context_creation_pipeline.py
102

in the short term, it would be nice to be able to set an environment variable like DAGSTER_RESOURCES_STRICT_MODE to override this; that would make it pretty easy to burn down the list of tests and ensure all resource requirements are specified

python_modules/dagster/dagster_tests/core_tests/test_strict_resources.py
274

maybe worth also adding a test raising an error when a solid tries to consume a resource it didn't specify as a requirement?

This revision is now accepted and ready to land.Dec 1 2019, 4:46 PM
schrockn added a comment.Dec 1 2019, 7:17 PM

Migrating the test suite is a pretty modest change: https://dagster.phacility.com/D1524

schrockn mentioned this in R1:c3994b5e927a: Check-in expected failure repros for #1950 and #1949.Dec 2 2019, 10:23 PM
schrockn marked an inline comment as done.Dec 2 2019, 10:25 PM
schrockn added inline comments.
python_modules/dagster/dagster_tests/core_tests/test_strict_resources.py
274

test_filter_out_resources does not. currently we don't raise a special error for that situation. I could change it so that the generated class has a special method that gives a nicer error message in that case.

schrockn added inline comments.Dec 2 2019, 10:35 PM
python_modules/dagster/dagster_tests/core_tests/test_strict_resources.py
274

test_filter_out_resources does this test, i meant

schrockn retitled this revision from Add strict mod to pipelines and only initialized required resources to Add strict mode to pipelines and only initialized required resources.Dec 2 2019, 10:40 PM
schrockn edited the summary of this revision. (Show Details)
alangenfeld added a comment.Dec 2 2019, 10:47 PM

should there be a test for multiprocess engine / execute plan APIs that ensures unused resources are not instantiated?

python_modules/dagster/dagster/core/definitions/decorators.py
820

still feels off to me to spec this here - feels like it at least belongs on mode next to the other resource information

python_modules/dagster/dagster/core/definitions/solid.py
391–392

if I add a new solid to a composite and don't specify its requirements (where previously all of them did) what failure case do i hit?

python_modules/dagster/dagster/core/definitions/system_storage.py
81

if were going to land this diff we should note it in changes.md so we dont forget

python_modules/dagster/dagster/core/execution/api.py
194

this is a little confusing since its just the non-composite solid names [1]

python_modules/dagster/dagster/core/execution/context_creation_pipeline.py
156

this argument feels really weird compared to the others

158

doubly odd since its very specific information that is just getting passed in to this sibling argument

python_modules/dagster/dagster/core/execution/plan/plan.py
319–320

[1]

alangenfeld added a comment.Dec 2 2019, 11:00 PM

Do we have an exact game plan for all this yet? I would push for deferring landing anything and trying to jump right to the end state unless there is a clear user need to satisfy.

schrockn added a comment.Dec 3 2019, 12:38 AM

👍🏻 @alangenfeld agree let's chat when you are in tmrw

alangenfeld requested changes to this revision.Dec 3 2019, 9:03 PM

from in person discussion:

  • move strict mode arg to mode (maybe rename it to describe the behavior of selective instantiation it turns on)
  • remove required_resource_keys from composite solid and resolve that change up through the graphql layer
  • explore cleaner options for providing the information currently handled by solid_def_names_in_execution
This revision now requires changes to proceed.Dec 3 2019, 9:03 PM
schrockn updated this revision to Diff 7308.Dec 8 2019, 4:40 PM

rebase

Harbormaster completed remote builds in B5904: Diff 7308.Dec 8 2019, 4:52 PM
schrockn added a comment.Dec 8 2019, 5:40 PM

@alangenfeld I've been trying to isolate what bothers me about attaching this behavior to mode. I think it is the following scenario.

You have two modes, one strict, one not.

The non-strict mode is used for local testing.

@solid
def something(context):
   context.resources.some_resource.it_works()

If you run this in a the non-strict local testing model it works fine as the resources are unconditionally created. However in strict mode some_resource would not be created and this error would only be caught at runtime. Seems pretty wrong to me. I think having this behavior attached to Mode actually allows for too much flexibility and can get a user into quite the confusing situation.

alangenfeld requested changes to this revision.Dec 10 2019, 10:25 PM

thats fair, though now that we're on the 0.7.0 train should we just skip the resources_strict_mode flag and just go for it?

thoughts on [A] & [B] from previous comments still hold

python_modules/dagster/dagster/core/definitions/solid.py
387–395

[A]

python_modules/dagster/dagster/core/execution/context_creation_pipeline.py
156

[B]

This revision now requires changes to proceed.Dec 10 2019, 10:25 PM
prha commandeered this revision.Jan 23 2020, 11:25 PM
prha added a reviewer: schrockn.
prha planned changes to this revision.Jan 24 2020, 12:33 AM
prha mentioned this in D1894: add required_resource_keys throughout examples code.Jan 27 2020, 10:08 PM
prha mentioned this in R1:00d1f4b74b6d: add required_resource_keys throughout examples code.Jan 27 2020, 10:43 PM
prha updated this revision to Diff 9011.Jan 28 2020, 1:51 AM
  • remove strict mode flag
  • add change log
  • add better error message
  • added composite resource test
  • switched solid resource argument to execution plan
  • removed NoResourceReqsSpecifiedSentinel
  • marked resources for airline_demo notebooks (HEAD -> prha/required_resources)
Harbormaster completed remote builds in B7324: Diff 9011.Jan 28 2020, 2:04 AM
alangenfeld accepted this revision.Jan 28 2020, 10:11 PM

I'm ok handling the clean-up refactoring in a follow up but lets make sure not to lose track of any of it

python_modules/dagster/dagster/core/definitions/resource.py
139–157

can drop this right

python_modules/dagster/dagster/core/definitions/solid.py
183–187

is there not opt_set_param? maybe add it?

python_modules/dagster/dagster/core/definitions/system_storage.py
81

make sure to mention this in changes.md

python_modules/dagster/dagster/core/errors.py
247–262 ↗(On Diff #9011)

do we have enough info on the context to know if the resource was on the mode def we are using?

python_modules/dagster/dagster/core/execution/context_creation_pipeline.py
150–170

should we refactor this to just thread through execution_plan since the info is all on there?

python_modules/dagster/dagster/core/execution/plan/plan.py
319–320

I am pretty sure this is wrong (not the impl of the function as its named but what its used for) but "fixing" this requires resource mapping working / to be fixed

322–323

think this could look more like [2] - double check the composition of these functions

335

[2]

This revision is now accepted and ready to land.Jan 28 2020, 10:11 PM
prha marked 8 inline comments as done.Jan 28 2020, 11:53 PM
prha added inline comments.Jan 29 2020, 12:27 AM
python_modules/dagster/dagster/core/errors.py
247–262 ↗(On Diff #9011)

We have enough info on the context, but we'd have to plumb that through to the ScopedResource object if we wanted to add a more descriptive error message. What did you have in mind?

python_modules/dagster/dagster/core/execution/context_creation_pipeline.py
150–170

I have this done in the next diff in the stack (factoring in the input_hydration_config for custom types)

prha updated this revision to Diff 9059.Jan 29 2020, 12:32 AM
  • comments from alex
Harbormaster completed remote builds in B7362: Diff 9059.Jan 29 2020, 12:44 AM
prha retitled this revision from Add strict mode to pipelines and only initialized required resources to Only initialize required resources during step execution.Jan 29 2020, 12:57 AM
prha edited the summary of this revision. (Show Details)
Closed by commit R1:7292dd6be326: Only initialize required resources during step execution (authored by prha). · Explain WhyJan 29 2020, 1:02 AM
This revision was automatically updated to reflect the committed changes.
prha added a commit: R1:7292dd6be326: Only initialize required resources during step execution.

Revision Contents
Changeset List

PathSize
examples/
dagster_examples/
airline_demo/
1 line
9 lines
2 lines
dagster_examples_tests/
airline_demo_tests/
unit_tests/
5 lines
python_modules/
dagster-graphql/
dagster_graphql/
schema/
14 lines
dagster/
dagster/
core/
definitions/
23 lines
4 lines
60 lines
21 lines
34 lines
13 lines
44 lines
execution/
20 lines
context/
4 lines
61 lines
plan/
3 lines
storage/
7 lines
utils/
3 lines
dagster_tests/
core_tests/
storage_tests/
2 lines
2 lines
234 lines
dagstermill/
dagstermill/
14 lines
37 lines
libraries/
dagster-aws/
dagster_aws/
s3/
1 line
dagster_aws_tests/
s3_tests/
18 lines
dagster-gcp/
dagster_gcp_tests/
gcs_tests/
18 lines

Diff 6975

View Options

examples/dagster_examples/airline_demo/pipelines.py

Loading...
View Options

examples/dagster_examples/airline_demo/solids.py

Loading...
View Options

examples/dagster_examples/airline_demo/unzip_file_handle.py

Loading...
View Options

examples/dagster_examples_tests/airline_demo_tests/unit_tests/test_load_data_from_spark.py

Loading...
View Options

python_modules/dagster-graphql/dagster_graphql/schema/solids.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/decorators.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/executor.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/mode.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/pipeline.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/resource.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/solid.py

Loading...
View Options

python_modules/dagster/dagster/core/definitions/system_storage.py

Loading...
View Options

python_modules/dagster/dagster/core/execution/api.py

Loading...
View Options

python_modules/dagster/dagster/core/execution/context/system.py

Loading...
View Options

python_modules/dagster/dagster/core/execution/context_creation_pipeline.py

Loading...
View Options

python_modules/dagster/dagster/core/execution/plan/plan.py

Loading...
View Options

python_modules/dagster/dagster/core/storage/system_storage.py

Loading...
View Options

python_modules/dagster/dagster/utils/test.py

Loading...
View Options

python_modules/dagster/dagster_tests/core_tests/storage_tests/test_system_storage_resources.py

Loading...
View Options

python_modules/dagster/dagster_tests/core_tests/test_mode_definitions.py

Loading...
View Options

python_modules/dagster/dagster_tests/core_tests/test_strict_resources.py

Loading...
View Options

python_modules/dagstermill/dagstermill/context.py

Loading...
View Options

python_modules/dagstermill/dagstermill/manager.py

Loading...
View Options

python_modules/libraries/dagster-aws/dagster_aws/s3/solids.py

Loading...
View Options

python_modules/libraries/dagster-aws/dagster_aws_tests/s3_tests/test_intermediate_store.py

Loading...
View Options

python_modules/libraries/dagster-gcp/dagster_gcp_tests/gcs_tests/test_intermediate_store.py

Loading...