Page MenuHomeElementl

[3/n init_resources] Resource initialization refactor
ClosedPublic

Authored by cdecarolis on Feb 16 2021, 8:38 PM.

Details

Summary

This diff refactors the existing code path for resource initialization.

  1. Makes various top level args Noneable on the resource initialization manager.
  2. Adds a new config wrapper around resource config.
Test Plan

Changed tests to reflect new resource config wrapper.

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Got rid of InitResourceContext artifacts, accept run config instead of env config

python_modules/dagster/dagster/core/execution/pre_execution_resources_init.py
1 ↗(On Diff #31877)

agreed. working title for sure

Fix run config passed in to init_resources from version resolution pathway

cdecarolis retitled this revision from [resource initialization 3/n] resource initialization pre-execution to resource initialization out-of-execution..Feb 23 2021, 4:11 PM
cdecarolis edited the summary of this revision. (Show Details)

Major refactor, reuse most of core resource init path

good work on the refactor this is close.

Trying to think of a better name for init_resources if this will become a top level API

python_modules/dagster/dagster/core/execution/context/init.py
16–47

can just keep this in order and make it optional / None to reduce change

python_modules/dagster/dagster/core/execution/pre_execution_resources_init.py
59–63 ↗(On Diff #32325)

something odd here about run_config arg not just being run_config passed through

python_modules/dagster/dagster/core/system_config/objects.py
13

what do you think about adding a wrapper class like this for the resources config section? I think it might help with discerning between raw config and processed resources section config

cdecarolis added inline comments.
python_modules/dagster/dagster/core/system_config/objects.py
13

ah I forgot that we did this for certain types of underlying config already. This definitely seems like the right approach, I agree that it's super weird that we just pull a dict off the env config and call it run config, bc it makes it seem as if it isn't validated.

Added ResourceConfig wrapper

Fixed tests to account for new resource mechanism

python_modules/dagster/dagster/core/execution/pre_execution_resources_init.py
54 ↗(On Diff #32518)

pull this out in to its own diff that includes tests / examples of how its useful

python_modules/dagster/dagster/core/execution/resolve_versions.py
15–25

is this a behavior change or clarification ? prefer to avoid sneaking behavior changes in to unrelated diffs

198–199

what?

python_modules/dagster/dagster/core/execution/resources_init.py
167–175

why is this event gated on this while the other are not?

200–206

this is a surprising amount of casting

python_modules/dagster/dagster/core/execution/resolve_versions.py
198–199

urgh need to get rid of this comment. My intention was that it's cleaner to accept a closed set of resources rather than accept all the resources on the mode and take a subset argument. Did not communicate that well

python_modules/dagster/dagster/core/execution/resolve_versions.py
15–25

Technically a behavior change will split it out.

python_modules/dagster/dagster/core/execution/resources_init.py
167–175

The others are already gated by resource_keys_to_init, which is always None in my code path

200–206

A few methods on execution plan have Optional[ExecutionStep] as return type, but we use them as if the step is confirmed not None.

cdecarolis retitled this revision from resource initialization out-of-execution. to [2/n init_resources] Resource initialization refactor.Mar 2 2021, 1:53 PM
cdecarolis edited the summary of this revision. (Show Details)
cdecarolis edited the test plan for this revision. (Show Details)
cdecarolis retitled this revision from [2/n init_resources] Resource initialization refactor to [3/n init_resources] Resource initialization refactor.Mar 2 2021, 4:00 PM

I think we may just need to explicitly encode some of the assumptions being made in [x] about the arguments. Are execution_plan and resource_keys_to_init expected to both be set or both be None?

python_modules/dagster/dagster/core/execution/resources_init.py
88–100

[x]

132–133

this change also adds to my confusion

167–175

i find this very subtle and confusing - if I was editing in this code in the future what is the behavior I should try to maintain?

Fixed arg on InitResourceContext

Added an argument to the resource generation sequence emit_persistent_events, to make it clear when certain events should be emitted.

Pull change into dagstermill

alangenfeld added inline comments.
python_modules/dagster/dagster/core/execution/resources_init.py
32–37

nit: [1]

183–188

nit [1]: should any of these arguments have default values? seems like being explicit at the callsites is good

This revision is now accepted and ready to land.Mar 3 2021, 7:52 PM
python_modules/dagster/dagster/core/execution/resources_init.py
183–188

yea that's a good point.

Don't provide default values for resource initialization