Page MenuHomePhabricator

(container-supported-dagit-3)-hook-container-context-into-cli
ClosedPublic

Authored by themissinghlink on Apr 7 2020, 6:13 PM.

Details

Summary

This revision adds an image flag and hooks in the DagsterContainerGraphQLContext to the dagit application.

Test Plan

dagit --image airline_user_code should just work and you should be able to explore the pipeline explorer page.

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

schrockn added inline comments.Apr 7 2020, 11:07 PM
python_modules/dagit/dagit/app.py
142–143

please refactor with less duplicated code

schrockn requested changes to this revision.Apr 7 2020, 11:10 PM
This revision now requires changes to proceed.Apr 7 2020, 11:10 PM

rebase and address feedback

  • rebased and added sanity test
schrockn requested changes to this revision.Apr 8 2020, 8:43 PM

minor changes but there seems to be a missing diff here? (e.g. the context rename)

python_modules/dagit/dagit/app.py
12

when did this rename happen?

204

where did this function come from?

207

when did we rename this?

python_modules/dagster-graphql/dagster_graphql/schema/roots.py
157

would rather see instanceof check here

This revision now requires changes to proceed.Apr 8 2020, 8:43 PM

Ugh.....this was an arc goof on my end. I am experimenting with the stacked diff approach (wheras before I would have made this 1 gigantic revision). May have messed up here.

themissinghlink planned changes to this revision.Apr 8 2020, 8:46 PM

Ah no the context rename happened in https://dagster.phacility.com/D2461. The reason for this was that it didn't make sense for the context to be about containers because it was really about passing in snapshots. Note that even in this revision, we are grabbing the container snapshot and then passing it into the context.

python_modules/dagit/dagit/app.py
204

This was done sometime back for windows support so I felt it prudent to leave it in. https://github.com/dagster-io/dagster/commit/17c8786a3f6c9f34890f90ba25703d93f7c695a3

schrockn accepted this revision.Apr 9 2020, 9:01 PM

cool

python_modules/dagit/dagit/app.py
204

oh i missed this this existed before. i thought this was func you had introduced.

This revision is now accepted and ready to land.Apr 9 2020, 9:01 PM