Page MenuHomePhabricator

Rewrite of the Composite Config Mapping and Parsing Stack
ClosedPublic

Authored by schrockn on Dec 13 2019, 5:12 PM.

Details

Summary

This is a substantial rewrite of the config mapping and parsing
stack.

The big conceptual change is that we have separated our config
processing into to two separate steps that can be cleanly composed,
and, most importantly, invoked recursively in a way with complexity
more managed.

We now have a core config type system that does not know anything
about the config mappings and the notion of solids at all. It is
just pure schema.

The manipulations around config mapping the descent into composite
solid definitions is now managed in a totally different pass,
currently called "composite_descent". It's only job in life is
to recurse down the config tree and construct a dictionary from
solid handles to SolidConfigs.

When it encouters a mapping function it dynamically constructs (this
should be cached) a config type that we expect to be returned from
the config mapping function, and this is validated using the
validate_config machinery that knows *nothing* about config mapping
construct at all. From the standpoint of that machinery, it is only
the *interface* (that is, the declared config field) into the config
mapping that is relevant.

The way to manage this in your is to note that there will be at minimum
1 validate_config call in a complete pass. And then there will be N
additional calls for the N instances of solids that are from
composite definitions with config mapping functions.

Test Plan

BK

Diff Detail

Repository
R1 dagster
Branch
new-config-parsing
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
Harbormaster failed remote builds in B6203: Diff 7676!
schrockn updated this revision to Diff 7696.Dec 13 2019, 7:44 PM
schrockn retitled this revision from extract validate step to Rewrite of the Composite Config Mapping and Parsing Stack.
schrockn edited the summary of this revision. (Show Details)
schrockn added reviewers: alangenfeld, nate.

upmessage

I still need to write nice error messages, but i would like to get thoughts on overall direction

looks good to me so far

python_modules/dagster/dagster/core/errors.py
196–197

πŸ˜„

python_modules/dagster/dagster/core/types/config/evaluator/composite_descent.py
20–30

Looks like you dont use solids really so I would just hold on to a solid handle. You may not even need this stack object and just be able to use a handle

class DescentStack:
    def __init__(self, pipeline_def, handle):
        self.pipeline_def = check.inst_param(pipeline_def, 'pipeline_def', PipelineDefinition)
        self.handle = check.inst_param(handle, 'handle', SolidHandle)

    @property
    def current_container(self):
        parent_handle = handle.parent
        return self.pipeline_def.get_solid(parent_handle) if parent_handle else self.pipeline_def

    def descend(self, solid):
        return DescentStack(
            self.pipeline_def, 
            SolidHandle(solid.name, solid.definition.name, self.handle)
        )
73

(ron howard voice): it was not

python_modules/dagster/dagster_tests/core_tests/config_types_tests/evaluator_tests/test_config_mappings.py
24–26

ya these can be updated now

schrockn added inline comments.Dec 13 2019, 10:55 PM
python_modules/dagster/dagster/core/types/config/evaluator/composite_descent.py
20–30

We need it for nice error messages. See raise_composite_descent_error

alangenfeld added inline comments.Dec 13 2019, 10:57 PM
python_modules/dagster/dagster/core/types/config/evaluator/composite_descent.py
161–164

ya I think you can get effectively the same information with just the handle

nate added inline comments.Dec 14 2019, 12:32 AM
python_modules/dagster/dagster/core/types/config/evaluator/composite_descent.py
73

+1 it was not

python_modules/dagster/dagster/core/types/config/evaluator/traversal_context.py
9–10

chefkiss

nate added a comment.Dec 14 2019, 1:25 AM

per our conversation, love the direction, this is a massive improvement.

I don't see anything thematically wrong here, full steam ahead! let me know when ready for a closer read.

python_modules/dagster/dagster/core/types/config/evaluator/validate.py
96

seeing this reminded me that this function is tough to reason about. not for this pass, but it would be great to refactor eventually

schrockn updated this revision to Diff 7730.Dec 14 2019, 5:57 AM

alex's feedback and improved error messages

schrockn updated this revision to Diff 7733.Dec 14 2019, 6:11 AM

old file for easier review

schrockn updated this revision to Diff 7742.Dec 14 2019, 5:05 PM

rebase for coverage fix

schrockn added inline comments.Dec 14 2019, 8:12 PM
python_modules/dagster/dagster/core/types/config/evaluator/validate.py
2

i'm going to move the body of the function to this file in subsequent diff. I just wanted to be able to see the diff of the old file in the review

schrockn updated this revision to Diff 7750.Dec 14 2019, 8:13 PM

Ready for real review now

schrockn updated this revision to Diff 7758.Dec 15 2019, 6:30 PM

rekickoff build

alangenfeld accepted this revision.Dec 16 2019, 4:45 PM

nice work

winclap

python_modules/dagster/dagster/core/types/config/evaluator/composite_descent.py
56

neato

202

via its config_mapping_fn

I don't think its actually called config_mapping_fn at user API level so this might be confusing

This revision is now accepted and ready to land.Dec 16 2019, 4:45 PM
schrockn updated this revision to Diff 7796.Dec 16 2019, 4:58 PM

alex feedback

This revision was automatically updated to reflect the committed changes.