Page MenuHomeElementl

[dagit] Crag: Populate empty config if only one preset
ClosedPublic

Authored by dish on Jun 30 2021, 8:59 PM.

Details

Summary

For Crag flagged users, in cases where there is only one preset for a given job, automatically populate the Playground config editor with that preset yaml and do not render the preset picker. Similarly, if there is only one partition set for the mode, do not render the partition set picker.

Test Plan

Clear localStorage.

With flag enabled:

  • View a pipeline:mode tuple Playground page with only one preset. Verify that the editor is populated with the preset config yaml.
  • Create a new tab on the Playground, verify that config is prepopulated.
  • View a pipeline:mode tuple Playground page with only one partition set. Verify that the partition set picker is not rendered.

With flag disabled, verify that the above scenarios match the current behavior/rendering in master.

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Check presets and partition sets for mode match

js_modules/dagit/packages/core/src/execute/ExecutionSessionContainer.tsx
150–154

By moving the useStorage call down a level in the React tree, we can guarantee that the Pipeline has already been retrieved in the parent component.

This means that when useStorage fails to find a session (because one has not yet been created for this pipeline) and creates one, we can use the preset information for the newly created session.

dish requested review of this revision.Jun 30 2021, 9:18 PM

This looks good! It looks like runConfigYaml isn't used down in the ConfigEditorConfigPicker but otherwise this seems like a great change

js_modules/dagit/packages/core/src/execute/ConfigEditorConfigPicker.tsx
268–278

It doesn't really matter and it's sort of out of the scope of this diff, but it'd be nice to push this useMemo and the pipelineMode filter up into the parent component (ConfigEditorConfigPicker) so that this component can just take configGenerators instead of all the props, and push the onSelect split into onSelectPartitionSet / onSelectPreset up as well.

js_modules/dagit/packages/core/src/execute/ExecutionSessionContainer.tsx
539

Reading through this, it actually doesn't look like this prop is used inside the ConfigEditorConfigPicker - I think this can be removed?

If we keep it, it might be nice to change this prop to something that more explicitly explains what it's used for - if we just need to compare against the current value in the session, it might actually be more clear to pass in the entire currentSession (since it gets 3 parts of it now), because the if statement could be like if preset === currentSession.runConfigYaml) etc

This revision is now accepted and ready to land.Jul 1 2021, 3:05 PM

+bengotow. Ended up moving and deleting quite a bit, so probably deserves a fresh review.

dish requested review of this revision.Jul 1 2021, 8:06 PM
dish marked 2 inline comments as done.

+1 for doing this. Resigning because I'm not qualified to comment on the implementation.

This looks great! Thanks for addressing the other stuff, I just added one comment about the presets.length == 1 && partitionSets case but I think this is good to go!

js_modules/dagit/packages/core/src/execute/ExecutionSessionContainer.tsx
146

This looks good! It slightly confused me that this logic is not quite the same as the logic around configGenerators >= 1 inside the ConfigEditorConfigPicker, but this one is for presets only.

I wonder if we need to add a check in here that presetsForMode.length === 1 AND that there are no partition sets? If the user had one preset and one partition set it might be odd for the editor to prefill.

This revision is now accepted and ready to land.Jul 6 2021, 12:48 PM
js_modules/dagit/packages/core/src/execute/ExecutionSessionContainer.tsx
146

Yeah, I think you're right.