Page MenuHomePhabricator

RFC: Eliminate ConfigMappingContext
ClosedPublic

Authored by sashank on Jan 14 2020, 10:26 PM.

Details

Summary

Surprisingly, after we removed execution_epoch_time, none of the examples throughout the codebase used the context on config_fn. This diff eliminates ConfigMappingContext entirely, since it seems like it's not useful anymore.

Relevant Issues:
https://github.com/dagster-io/dagster/issues/2022
https://github.com/dagster-io/dagster/issues/2023

Test Plan

unit

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

sashank created this revision.Jan 14 2020, 10:26 PM
sashank edited the summary of this revision. (Show Details)Jan 14 2020, 10:27 PM
sashank edited the summary of this revision. (Show Details)Jan 14 2020, 10:33 PM
sashank added reviewers: schrockn, alangenfeld.
sashank retitled this revision from Eliminate ConfigMappingContext to RFC: Eliminate ConfigMappingContext.

I'm into it. Add entry to CHANGES.md?

sashank added inline comments.Jan 14 2020, 10:39 PM
python_modules/dagster/dagster/core/system_config/composite_descent.py
35–37

1/2 Most relevant changes - we remove the context on the descent stack

156

2/2 Most relevant changes - We don't pass in the context to the mapping function

sashank updated this revision to Diff 8713.Jan 14 2020, 10:50 PM

Add breaking change to CHANGES.md

sashank updated this revision to Diff 8716.Jan 14 2020, 11:00 PM

make black (always forget to do this after a codemod)

it seems like something bad is happening in changed.md thats causing the autoformatter to go nuts

CHANGES.md
29–41

is this right? this seems like it got fucked up

29–46

?

388

?

alangenfeld added inline comments.Jan 15 2020, 10:19 PM
python_modules/dagster/dagster/core/definitions/config.py
45–50

hmmmm - there is some option value were giving up here. Having a hard time thinking of what we would want to pass through here though

sashank added inline comments.Jan 15 2020, 11:39 PM
CHANGES.md
29–46
sashank updated this revision to Diff 8726.Jan 15 2020, 11:56 PM

Rebase on CHANGES.md fix

sashank updated this revision to Diff 8727.Jan 15 2020, 11:58 PM

Try CHANGES.md fix again

Harbormaster failed remote builds in B7079: Diff 8727!
schrockn accepted this revision.Jan 16 2020, 5:31 PM
schrockn added inline comments.
python_modules/dagster/dagster/core/system_config/composite_descent.py
28–29

trailing space in string

37

happycry

This revision is now accepted and ready to land.Jan 16 2020, 5:31 PM
sashank updated this revision to Diff 8762.Jan 16 2020, 11:48 PM

remove trailing space

sashank updated this revision to Diff 8764.Jan 17 2020, 12:37 AM

Actually isort

This revision was automatically updated to reflect the committed changes.