Page MenuHomeElementl

Improved error message when missing config
ClosedPublic

Authored by owen on Apr 13 2021, 11:05 PM.

Details

Summary

This error message is displayed the same in both dagit and the CLI. I decided to render the expected format as a python dictionary (essentially JSON) instead of YAML, as I'd imagine that if you are seeing this error, it's more likely that you will be fixing the issue in python than in a yaml file.

Example 1: In the dagit playground, there is already a "scaffold missing config" button, so the fact that you're editing yaml and the error produced JSON is probably not a big deal (generally the playground flows are very nice, and don't require me to carefully read the specific error messages)

Example 2: You see this error for a sensor in dagit, you'll likely have the sensor config function defined using python dictionaries.

Example 3: You see this error while writing a test, you probably just have an execute_pipeline(my_pipeline) call that you can just copy-paste the expected config from the error into.

Of course, there are always cases where you'll be loading a config.yaml in python, and so this format won't be as ergonomic, but it still presents a huge upgrade over not showing anything at all.

github issue: https://github.com/dagster-io/dagster/issues/3991

Right now, if you pass in an empty config object and try to execute a pipeline that has some required solid config, you get a fairly unhelpful messaage "Missing required config entry "solids" at the root". This isn't great, because after this error message you still don't have a particularly fast route to fixing the issue. It doesn't tell you what solid needs config, and what config that solid needs.

When I see this error message, generally I am looking for the minimum set of config needed to run this pipeline. I don't want to keep expanding the config one error at a time (run_config={"solids":{}}, run_config={"solids":{"my_config_solid":{}}}, etc.).

Note that while this realistically does essentially copy-paste the CLI implementation of the scaffolding config,

  1. the functionality is actually different in how it treats scalar union types (CLI ignores, we choose the first of the union types)
  2. I am not the original sinner here (in terms of reimplementing this functionality). https://sourcegraph.com/github.com/dagster-io/dagster/-/blob/js_modules/dagit/packages/core/src/execute/scaffoldType.ts

It might not be a bad idea to unify this logic a bit better, but I'm not aware of if there are reasons that the CLI tool doesn't scaffold scalar union types (for example)

Test Plan

bk

Diff Detail

Repository
R1 dagster
Branch
invalid_config_error_message
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

added alternative way to include error info

Status quo for some common errors:

Screen Shot 2021-04-14 at 12.23.50 PM.png (418×730 px, 62 KB)

After changes:

Screen Shot 2021-04-14 at 12.21.45 PM.png (424×714 px, 68 KB)

Considering swapping out the format when it's displayed in dagit

owen published this revision for review.Apr 16 2021, 4:56 PM
owen edited the summary of this revision. (Show Details)

This will be a huge approachability and general usability win.

python_modules/dagster/dagster/config/errors.py
223

Nitpick: no newline needed at beginning of function

227

If I understand correctly, "scaffold" here has a subtly different meaning than how we use scaffold elsewhere? Would it make sense to call this minimal_config_for_type_snap or something?

python_modules/dagster/dagster/config/snap.py
221

A return type annotation would be helpful here.

240

Do we need to handle Float?

i think this is a clear step forward even if its json sample in yaml land dagit

also - looks like we are not including commas between fields for the "expected" section when there is extra config? not sure if thats an easy diff to throw out while you're in this zone

resolve inlines before landing

python_modules/dagster/dagster/config/snap.py
241

maybe ...for string

This revision is now accepted and ready to land.Apr 16 2021, 6:09 PM
owen marked 5 inline comments as done.

up

i think this is a clear step forward even if its json sample in yaml land dagit

also - looks like we are not including commas between fields for the "expected" section when there is extra config? not sure if thats an easy diff to throw out while you're in this zone

resolve inlines before landing

re the comma issue -- took a crack at it, but due to the very fancy recursive way that those types get printed, it's actually fairly hard to place commas in a sensible way (maybe we should make this an interview question and get someone to do the dirty work for us 😛 )

  • restored correct version of file
This revision was automatically updated to reflect the committed changes.