Page MenuHomeElementl

UI support for dynamic outputs, Run view state cleanup
ClosedPublic

Authored by bengotow on Jan 23 2021, 8:15 PM.

Details

Summary

This diff exposes the "isDynamic" property on the OutputDefinition GraphQL interface, and uses it in the Pipeline > Overview tab to show a "1-to-many" (*) on outputs that are dynamic on the DAG view and the sidebar.

The Run details page has been refactored quite a bit - previously, this view pulled in the execution plan for the run and the run logs, but they were used side by side. In order to support dynamic outputs, we need to assemble the metadata from the logs (eg: step keys), and transform the execution plan to reflect the actual invocations of the dynamic steps (by duplicating unresolved sub-graphs).

In addition to shifting the data around a bit, I also had to change the way StepSelection is persisted on the Run page. Previously we kept both the user input "query" and the resolved "step keys" in state. Because the query => keys transform is dependent on the keys discovered in the logs at runtime it's now computed rather than stored. I think this actually made a bunch of stuff cleaner!

I also updated the Run tab to useQueryPersistedState for the page state instead of the bespoke url->state and copy-to-clipboard functionality. I think the old implementation used URLSearchParams nicely though and in a separate diff we may want to explore using that class within the standard hook to serialize array values into the query string more cleanly.

Note:
There are a few places Dagit is still pulling apart step keys and relying on the [?] style syntax. I think this is actually unavoidable because it needs to transform the execution plan based on the step keys it discovers at runtime and needs to understand how to do the replacement. To keep things as clean as possible though, this all runs through helper functions in DynamicStepSupport.tsx.

image.png (324×534 px, 30 KB)

image.png (208×296 px, 13 KB)

image.png (1×1 px, 240 KB)

Test Plan

Run new tests!

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 23 2021, 8:34 PM
Harbormaster failed remote builds in B24774: Diff 30170!

Update python tests + snapshots

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 23 2021, 10:07 PM
Harbormaster failed remote builds in B24776: Diff 30172!

Update more python snapshots

Very excited to see this land. Will defer to dish on the deets of the js.

defer to @dish for final

js_modules/dagit/src/gantt/DynamicStepSupport.tsx
2–16

🙃

this seems reasonable enough - we can definitely add GraphQL schema to remove the need for this but this seems landable

probably worth a comment block somewhere in this file about the assumptions taken

python_modules/dagster/dagster/core/snap/solid.py
74–305

looks good

dish added inline comments.
js_modules/dagit/src/gantt/DynamicStepSupport.tsx
4

Just checking, there's no danger of a step ever having brackets in its name, right?

js_modules/dagit/src/hooks/useQueryPersistedState.tsx
17

Why this instead of the ref?

This revision is now accepted and ready to land.Jan 25 2021, 11:45 PM
bengotow added inline comments.
js_modules/dagit/src/gantt/DynamicStepSupport.tsx
2–16

God call, will add a block comment to this file explaining the use 👍

js_modules/dagit/src/hooks/useQueryPersistedState.tsx
17

Hey hey! This is an interesting case actually... if you have multiple useQueryPersistedState's defined in your component, and call both setters back to back, it's not possible for them to both have the up-to-date querystring without it being global I think.

Eg:

const [a, setA] = useQueryPersistedState(...);
const [b, setB] = useQueryPersistedState(...);

onClick={() => {
setA(1);
setB(2);
}

If we use a ref inside each of the hooks, their values are updated on render, but React runs the render after both setters are called so the second setter is operating on stale data. (eg: location.pathname and qs.current both reflect the old data)

I'm not super happy with this solution, would definitely be curious if there's a better way?

Rebase and address diff feedback

This revision is now accepted and ready to land.Jan 31 2021, 5:17 PM