Page MenuHomePhabricator

RFC: handle type-based loading with input managers
AbandonedPublic

Authored by sandyryza on Tue, Dec 22, 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 OK
Unit
No Unit Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Tue, Dec 22, 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
203

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

316

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

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

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

python_modules/dagster/dagster/core/execution/context/system.py
537–545

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
203–204

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

316

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.Tue, Jan 5, 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.Thu, Jan 7, 5:07 PM