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.
Details
Details
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
Diff Detail
- Repository
- R1 dagster
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| 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. | |
Comment Actions
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 | |
Comment Actions
+bengotow. Ended up moving and deleting quite a bit, so probably deserves a fresh review.
Comment Actions
+1 for doing this. Resigning because I'm not qualified to comment on the implementation.
Comment Actions
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. | |
| js_modules/dagit/packages/core/src/execute/ExecutionSessionContainer.tsx | ||
|---|---|---|
| 146 | Yeah, I think you're right. | |