Page MenuHomePhabricator

Refactor configured infrastructure to eliminate "underscore" params on public definitions
ClosedPublic

Authored by schrockn on Nov 24 2020, 6:54 PM.

Details

Summary

As I've been exploring the notion of
graph/composite_solid/pipeline and the notion of consolidating a bunch
of concepts into the resource system, I keep on running into the
configured stuff, which has some pretty rough edges.

In particular I think the inclusion of these internal parameters on public APIS
to implement the configured pattern is quite problematic.

Instead of checking for the recursion by passing around an opaque
closure that contains back pointers to the parent definition it its
implementation, instead we just construct a lineage of configuration
schemas as a data structure, and pass that data structure in where
config schema used to be allowed. The config evaluation then traces this
data structure. This allows us to reuse the config_schema param for
this purpose.

Although this does not do this, I am looking forward to potentially
completely merging the ConfigMapping notion on Graph with configured.
They are both stacks of config expansion and should share more code.

Test Plan

BK

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 24 2020, 7:12 PM
Harbormaster failed remote builds in B21684: Diff 26319!
Harbormaster returned this revision to the author for changes because remote builds failed.Nov 25 2020, 6:52 PM
Harbormaster failed remote builds in B21781: Diff 26455!
Harbormaster returned this revision to the author for changes because remote builds failed.Nov 25 2020, 7:21 PM
Harbormaster failed remote builds in B21784: Diff 26458!
Harbormaster returned this revision to the author for changes because remote builds failed.Nov 25 2020, 8:43 PM
Harbormaster failed remote builds in B21790: Diff 26466!
schrockn retitled this revision from Developing things to Refactor configured infrastructure to eliminate "underscore" params on public definitions.
schrockn edited the summary of this revision. (Show Details)

up

seems like a reasonable step forward - do you have any thoughts to jot down about how to collapse ConfigMapping in to this functionality as inline comments? In case you are not able to finish that and someone else tries later. [1] as places to maybe add notes

python_modules/dagster/dagster/core/definitions/pipeline.py
252–255

[1]

This revision is now accepted and ready to land.Nov 30 2020, 5:20 PM

Yeah I honestly don't have too many super profound thoughts.

The codepath in EnvironmentConfig.build feels like it should be a more cohesive recursive descent across all potential configurable definitions (resources + solids + system resources) that do a mapping step, but I haven't given it too much thought beyond that. Not sure if that is coherent enough to leave as as comment.