Page MenuHomePhabricator

Replace playground subset selector with one that leverages solid query syntax
ClosedPublic

Authored by bengotow on Jan 8 2020, 8:33 PM.

Details

Summary

This diff modifies the playground's solid subset selector to use the solid query syntax and the same rules for rendering / not-rendering (*), which fixes an issue encountered opening this modal on large DAGs.

It also fixes an issue where the DAG would render off in a corner of the modal, which was caused by the dialog's animation changing the SVGViewport's perceived size.

Under the hood, we're still converting the selection to a solid name array to pass to execution. It'd be nice to eliminate the solidSubset:[] param and use solidQuery everywhere, but we can implement the backend portion separately.

Test Plan

Run snapshot tests

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 created this revision.Jan 8 2020, 8:33 PM
bengotow edited the summary of this revision. (Show Details)Jan 8 2020, 9:56 PM
prha requested changes to this revision.Jan 8 2020, 10:20 PM
prha added inline comments.
js_modules/dagit/src/execute/SolidSelector.tsx
111โ€“113

I think this allows you to persist an invalid query, which is confusing when we're storing the query as the button name...

e.g. when my query is garbageoaweijfoaweijfowj, that's the selector even though the solids subset matches *.

We might be better off about more aggressively checking the query and disabling the Apply button so that the save is not actually called?

It's especially confusing when I click Apply with the input field completely blank.

151

can we add 'space' to the key events that trigger onChange?
can we trigger it onBlur?

Trying to avoid the situation where the query I'm "applying" isn't reflected in the solid viewer.

151

I'd love to have spaces be a viable delimiter for the query string

e.g. A B C is equivalent to A, B, C

What do you think?

This revision now requires changes to proceed.Jan 8 2020, 10:20 PM
bengotow planned changes to this revision.Jan 13 2020, 2:45 PM
bengotow added inline comments.
js_modules/dagit/src/execute/SolidSelector.tsx
111โ€“113

Hmm I agree we shouldn't let you exit the modal with an invalid subset matching zero solids. Will fix this!

The garbageoaweijfoaweijfowj matches no solids and sends [] to the server, but it looks like it's interpreting that as "subset not provided" and matching the entire pipeline.

151

Yep it commits on blur ๐Ÿ‘ I do need to check that in this modal format the blur is handled before the Apply button click though.

I'm fine with space being allowed, will update the filter logic a bit.

bengotow updated this revision to Diff 8636.Jan 13 2020, 3:16 PM
  • Allow space as a delimiter in query syntax
  • Only allow user to submit dialog when the query is valid
  • Blur the active field when firing a shortcut so the query is committed
alangenfeld resigned from this revision.Jan 13 2020, 4:15 PM

let @prha take this one

prha accepted this revision.Jan 13 2020, 4:54 PM

This feels much better!

We should still think about if we want to allow garbage in the selector query as long as a solid is in the subselection (e.g. is garbageoaweijfoaweijfowj, legit_solid_name a valid query?) , but not a blocker...

This revision is now accepted and ready to land.Jan 13 2020, 4:54 PM

Hmm that's a good point re: partially invalid queries โ€” it seems every portion of the query should be required to match at least one solid. Until we can display errors in context in the field, we should probably err on the side of it being valid though.