Page MenuHomePhabricator

Change partition matrix click behavior, split large files and GraphQL types
ClosedPublic

Authored by bengotow on Oct 26 2020, 2:33 AM.

Details

Summary

This diff expands on the partition matrix's "click to show runs" behavior so that clicking a step in a particular day shows you all the runs of that day annotated with the state of that particular step and a link to go directly to the logs with the step filter applied.

In the spirit of trying to load as little data as possible, the UI no longer loads the data required for the RunTable until you click on a particular step and open the runs modal. To render the table with the additional column showing the stats of the clicked step, it blends the data from the original query (which fetched all the per-step stats, but less info about the runs), with the full run data. This is /slightly/ awkward, but the API doesn't provide us with a great way to fetch the runs with stepStats about a single step. I am also using the partition run tag to query for the runs, which may be the first time Dagit is assuming the existence of that tag.

Loom: https://www.loom.com/share/42535660301f4e63bb0462ab24e4b546

I also spent some time cleaning up the Partition view React components to make it easier to do more performance related refactoring soon. In retrospect it cluttered this diff a bit but it should make subsequent diffs smaller and more isolated I think!

  • I moved some supporting custom components to their own files in src/partitions. I'm a fan of keeping small custom components in the same file until they're used elsewhere, but PartitionView was getting out of hand.
  • I gave the partition matrix and the graphs on the page distinct GraphQL fragments and rolled them up into the root query so that it's easier to trace where fields we're fetching are used. This revealed a few fields we weren't using and a few that are only needed for the pipeline-wide expectation results + materializations graphs, which we might cut soon.
Test Plan

Kick the tires! Will write storybooks for PartitionMatrix this week

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

bengotow edited the summary of this revision. (Show Details)
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 26 2020, 2:41 AM
Harbormaster failed remote builds in B20125: Diff 24423!
bengotow edited the summary of this revision. (Show Details)

Rebase, merge with @prha's cleanup! πŸ™Œ

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 26 2020, 3:11 AM
Harbormaster failed remote builds in B20126: Diff 24424!
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 27 2020, 5:14 PM
Harbormaster failed remote builds in B20229: Diff 24546!
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 27 2020, 5:54 PM
Harbormaster failed remote builds in B20235: Diff 24552!
Harbormaster returned this revision to the author for changes because remote builds failed.Oct 27 2020, 8:46 PM
Harbormaster failed remote builds in B20265: Diff 24585!

I don't think I'm qualified to comment on the code, but I love the result!

I would push lightly against including the expectation count / materialization count. Just for the sake of reducing clutter. Though maybe I'm missing some use for them that you had in mind?

This revision is now accepted and ready to land.Oct 27 2020, 10:36 PM

Yeah, the flow of this is pretty nice.

There's probably a way to further extract some generic component out of the graphs, but maybe beyond the scope of this diff.

js_modules/dagit/src/partitions/PartitionGraphSet.tsx
130

This RunGraphFragment is probably a mistake....

I thought that we'd have two components, RunGraph and PartitionGraph that could be used to show by partition and by execution time respectively in the AssetView. But then we kind of rolled a custom component for that so now the hierarchy is a little muddled.

RunGraphUtils was a misnamed set of utilities to get the shared fragments/util functions for both.

js_modules/dagit/src/runs/RunTable.tsx
22

This is kind of cool!

js_modules/dagit/src/runs/RunTable.tsx
44–45

This is going to need a rebase in order to get the table change. Sorry about that!

I don't think I'm qualified to comment on the code, but I love the result!

I would push lightly against including the expectation count / materialization count. Just for the sake of reducing clutter. Though maybe I'm missing some use for them that you had in mind?

Thanks for the feedback folks! re: your question @sandyryza , I was vaguely thinking someone might want to look at the expectation results of the step on each run to investigate which run on a day was "the bad one" if there wasn't an explicit step failure. Maybe we can ask prezi this week and remove them if there's no validity to that, I agree they clutter the UI but I do think we should surface everything we can to avoid folks having to go to the logs for each run.

This revision is now accepted and ready to land.Oct 30 2020, 7:53 PM

Remove RunGraph class, rename RunGraphUtils > PartitionGraphUtils to reflect the current state of the system, we will reconsider base class / common types for graphs but for now it's more important that the requested types reflect exactly the data we need.

I agree they clutter the UI but I do think we should surface everything we can to avoid folks having to go to the logs for each run.

Cool - I trust your intuition on that,