Page MenuHomeElementl

RFC: handle type-based loading with input managers
AbandonedPublic

Authored by sandyryza on Dec 22 2020, 6:40 PM.

Details

Summary

This diff asks the question: "what if we got rid of the loader attribute on DagsterType and instead handled type-based loading inside input managers?"

A couple notable things:

  • Dynamic config schema - the per-input config schema provided by the input manager definition depends on the InputDefinition of the input.
  • Now, every InputDefinition has a default manager key - "input_manager". The default resource def provided for "input_manager" is an input manager that knows how to load primitive types from config and defers to the upstream object manager otherwise. Without this, type-based input-loading would not work out of the box.

The test cases at the end of the diff provide a comparison of the old and new user experience. I can also explore some more complex situations, but wanted to make sure the basic one is palatable.

Test Plan

will fail tests

Diff Detail

Repository
R1 dagster
Branch
dynamic-config-schema (branched from master)
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 22 2020, 6:55 PM
Harbormaster failed remote builds in B23342: Diff 28393!
sandyryza retitled this revision from handle type-based loading with input managers to RFC: handle type-based loading with input managers.
sandyryza edited the summary of this revision. (Show Details)
sandyryza added a reviewer: alangenfeld.
sandyryza edited the summary of this revision. (Show Details)
python_modules/dagster/dagster/core/execution/plan/inputs.py
177

The changes here fix an independent bug that was surfaced through these changes.

304

This fixes an independent bug that was surfaced through these changes.

python_modules/dagster/dagster_tests/core_tests/storage_tests/test_input_manager.py
195

Probably we'd replace this with something called just "loader".

python_modules/dagster/dagster/core/execution/context/system.py
530–538

btw one thing we can do to make this feel less awful is bucket all the things that are required *except* in the SolidExecutionResult into a single struct where that struct itself is Noneable but it is has required fields. Would dramtically reduce the possible state space of the InputContext

python_modules/dagster/dagster/core/execution/plan/inputs.py
177–178

strongly would consider extracting the unrelated bug fixes into their own diff

304

same same

to your queue post discussion, summary was roughly trying to simplify things a bit by not having overlapping responsibilities between input and object managers, and moving towards a RootInputManager set up where it is only responsible for root loads

This revision now requires changes to proceed.Jan 5 2021, 7:42 PM

can you clarify what you want feedback on at this stage? if there isn't any RFCish stuff left prep this for final review

This revision now requires changes to proceed.Jan 7 2021, 5:07 PM