Page MenuHomePhabricator

Improved support for displaying run storage with non-happy path states in dagit
ClosedPublic

Authored by max on Sep 13 2019, 11:37 PM.

Details

Summary

This handles the following cases in a preliminary way:

  1. Logs are present for a run whose pipeline cannot be found in the current repository.
  2. Logs are present for a run that failed without starting (e.g. due to a frameqwork python error)

It notably does *not* yet address https://github.com/dagster-io/dagster/issues/1735.

Test Plan

Manual, tbh.

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

max created this revision.Sep 13 2019, 11:37 PM
max edited the summary of this revision. (Show Details)Sep 13 2019, 11:39 PM
max updated this revision to Diff 4755.Sep 13 2019, 11:39 PM
max edited the summary of this revision. (Show Details)

Blk

Harbormaster failed remote builds in B3821: Diff 4755!
max updated this revision to Diff 4757.Sep 13 2019, 11:52 PM

snaps

prha added a subscriber: prha.Sep 14 2019, 12:19 AM
prha added inline comments.
js_modules/dagit/src/PipelineJumpComponents.tsx
143

When do you hit this state? When I enter an gibberish pipeline name, I get a GraphQL error, not a user-facing message:

js_modules/dagit/src/plan/ExecutionPlan.tsx
82

remove

125

remove

max updated this revision to Diff 4768.Sep 16 2019, 5:07 PM

undebug

max added inline comments.Sep 16 2019, 5:45 PM
js_modules/dagit/src/PipelineJumpComponents.tsx
143

this is when you have a run in run storage for a pipeline that isn't in the currently loaded repository

max edited the summary of this revision. (Show Details)Sep 16 2019, 5:49 PM
max added a comment.Sep 16 2019, 6:34 PM

Spurious build failures resolved on retry.

alangenfeld requested changes to this revision.Sep 17 2019, 4:34 PM

is that stack trace @prha got fixed in the current form of the diff? Are there dagster-graphql tests against the unknown pipeline behavior. If not it would be good to add some.

To your queue to add a tests that encode assumptions around PipelineOrError and UnknownPipeline (or request review if they are already covered)

js_modules/dagit/src/PipelineJumpComponents.tsx
143

this stack trace makes me believe the graphql schema change may have effected other fields

python_modules/dagster-graphql/dagster_graphql/implementation/fetch_pipelines.py
13–30

lets add some comments here since the behavior is relatively subtle

This revision now requires changes to proceed.Sep 17 2019, 4:34 PM
max added inline comments.Sep 17 2019, 4:48 PM
js_modules/dagit/src/PipelineJumpComponents.tsx
143

I see, phil just needs to rebuild dagit

max updated this revision to Diff 4860.Sep 18 2019, 10:20 PM

Rebase

max updated this revision to Diff 4861.Sep 18 2019, 10:23 PM

Rebase

max updated this revision to Diff 4864.Sep 18 2019, 10:32 PM

Rebase

max updated this revision to Diff 4867.Sep 18 2019, 10:43 PM

Fixes

max updated this revision to Diff 4869.Sep 19 2019, 1:01 AM

Fixes

max added a comment.Sep 19 2019, 1:02 AM

Indeed, added tests

alangenfeld added inline comments.Sep 19 2019, 3:26 PM
js_modules/dagit/src/runs/Run.tsx
54–63

since the return type of the pipeline field is PipelineReference - you shouldn't need to use a fragment here - just

pipeline {
  __typename
  name
  ... on Pipeline {
js_modules/dagit/src/schema.graphql
758

are we sure we want to make this change? I can see value in the pipeline root field still only returning a Pipeline if it is in scope other wise error

alangenfeld accepted this revision.Sep 19 2019, 5:29 PM

should be good to go after those two things

makeitso

This revision is now accepted and ready to land.Sep 19 2019, 5:29 PM
max updated this revision to Diff 4877.Sep 19 2019, 5:29 PM

Rebase

max updated this revision to Diff 4880.Sep 19 2019, 6:24 PM

Cleanup

alangenfeld added inline comments.Sep 19 2019, 6:37 PM
js_modules/dagit/src/PipelineExplorerRoot.tsx
114–118

clean this up

js_modules/dagit/src/runs/Run.tsx
56–58

*

js_modules/dagit/src/runs/RunHistory.tsx
216–223

*

js_modules/dagit/src/runs/RunRoot.tsx
51–53

*

python_modules/dagster-graphql/dagster_graphql_tests/execute.graphql
7

*

python_modules/dagster-graphql/dagster_graphql_tests/graphql/execution_queries.py
95–140

*

python_modules/dagster-graphql/dagster_graphql_tests/test_cli.py
93

*

max updated this revision to Diff 4882.Sep 19 2019, 7:10 PM

Black