Page MenuHomePhabricator

RFC: push config obj instantiation into `reconstitute_pipeline_context`
ClosedPublic

Authored by prha on Aug 15 2019, 11:00 PM.

Details

Summary

We want to minimize the code that is injected into dagstermill notebooks.

Test Plan

ran tests, ran through airline_demo, viewed output notebook in jupyter

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

prha created this revision.Aug 15 2019, 11:00 PM
prha added a reviewer: Restricted Project.Aug 15 2019, 11:05 PM

I'd like someone to sanity check my use of different library imports (six / seven / json).

For example, I'm not sure why six.raise_from was reimplemented in the injected parameters. There's probably something I'm missing about the different runtime environments?

prha planned changes to this revision.Aug 15 2019, 11:11 PM
prha updated this revision to Diff 3761.Aug 15 2019, 11:43 PM

fix python 2.7 syntax error

prha removed a reviewer: Restricted Project.Aug 15 2019, 11:44 PM
prha added a reviewer: Restricted Project.Aug 15 2019, 11:52 PM

This looks great to me. I'll let @max approve to resolve your question re: the use/non-use of raise_from.

Could you post a screenshot of the output notebook with this change applied?

alangenfeld resigned from this revision.Aug 16 2019, 9:36 PM
alangenfeld added a subscriber: alangenfeld.

defering to max
cleanup

max accepted this revision.Aug 16 2019, 10:13 PM
max added inline comments.
python_modules/dagstermill/dagstermill/translator.py
18–19

i wonder if we should go whole hog and give this a name like {uuid}_dagstermill

This revision is now accepted and ready to land.Aug 16 2019, 10:13 PM
prha added inline comments.Mon, Aug 19, 6:32 PM
python_modules/dagstermill/dagstermill/translator.py
18–19

i think i'd probably hold off until it's clearly necessary, because it's already scoped