Page MenuHomePhabricator

Events API
ClosedPublic

Authored by schrockn on Jun 15 2019, 6:22 PM.

Details

Summary

This is a pretty significant change to the events API, starting
with ExpectationResult and Materialization. Their APIs are now very
similar to eachother (identical except for an additional success bool
on ExpectationResult). The notable additional are of the "metadata
entries" of which currently their are four types: value, path, url, and
json. When the user emits one of these:

yield Materialization(
    label='Table: {table_name}'.format(table_name=table_name),
    description=(
        'Persisted table {table_name} in database configured in the db_info resource.'
    ).format(table_name=table_name),
    metadata_entries=[
        EventMetadataEntry.text(label='Host', text=context.resources.db_info.host),
        EventMetadataEntry.text(label='Db', text=context.resources.db_info.db_name),
    ],
)

It should be very clear what will be rendered in dagit. I'm imagining a
two-level display hierarhcy where each metadata entry corresponds to a
row.

This should be easy to extend in the future. Want to add a histogram
rendered? Add an EventMetdataEntry for a collection of numbers, add a
GraphQL type, and then add frontend support.

I did change the GraphQL schema, but I shoehorned the new data into the
existing client-side APIs (thank you Typescript!)

Looking for feedback as this is a substantial change.

Addresses https://github.com/dagster-io/dagster/issues/1408
Addresses https://github.com/dagster-io/dagster/issues/1438

Test Plan

Run things in dagit and look at them. Buildkite

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

schrockn created this revision.Jun 15 2019, 6:22 PM
schrockn updated this revision to Diff 2045.Jun 15 2019, 8:13 PM
schrockn edited the summary of this revision. (Show Details)

up

schrockn updated this revision to Diff 2046.Jun 15 2019, 8:22 PM
schrockn edited the summary of this revision. (Show Details)
schrockn edited the test plan for this revision. (Show Details)

up

schrockn updated this revision to Diff 2050.Jun 15 2019, 11:29 PM

Self review

max added a comment.Jun 17 2019, 12:30 PM

Very into pushing the types down like this. I think ExpectationResult.passed *is* probably boolean, but we are going to need another layer of config / display control. In the simplest case, we want to be able to configure whether execution should halt/error on the failure of an expectation (https://github.com/dagster-io/dagster/issues/1375), since sometimes we will want to display failed expectations in the UI or alert on them, but keep processing data.

python_modules/dagster/dagster/core/definitions/events.py
27

confusing, "if no label is provided"

99

is this going away?

bengotow accepted this revision.Jun 17 2019, 3:45 PM

This looks great! I like that this is generic and that we're also getting value type data from the backend (paths, json, etc.) so we don't have to do any guesswork when it comes to presenting the values. Good stuff!

python_modules/dagster/dagster/core/definitions/events.py
36

Was surprised to see that in the GraphQL layer there are multiple types at the EventMetadataEntry layer, and here in the Python there's a common wrapper which contains different "data" classes. Offhand I'm not sure I see the value of having EventMetadataEntry containing a JsonMetadataEntryData vs having JsonEventMetadataEntry subclass EventMetadataEntry (which would be more consistent with the GraphQL layer I think?) Either way it's not a big deal, just surprising since I saw the GraphQL first.

151

I like the idea of EventMetadataEntry being a sort of class cluster, where these helper methods hide the construction of the concrete subclasses + data classes so you don't have to import them all. I wonder if this could be shortened to EventMetadataEntry.json(label='json', value=result_metadata)? Calling the json value result_metadata seems very ExpectationResult specific and presumably this line would be the same for all events?

This revision is now accepted and ready to land.Jun 17 2019, 3:45 PM
schrockn updated this revision to Diff 2078.Jun 17 2019, 3:46 PM

comment on legacy_ctor

schrockn added inline comments.Jun 17 2019, 3:56 PM
python_modules/dagster/dagster/core/definitions/events.py
99

Yes. Just didn't want to change all callsites in one shot.

schrockn added inline comments.Jun 17 2019, 4:08 PM
python_modules/dagster/dagster/core/definitions/events.py
36

Chatted in slack

151

Yeah this is just a legacy construct to match old behavior to avoid migrating all callsites in one shot.

I'll make the the EventMetadataEntry json entry more concise ๐Ÿ‘๐Ÿป

schrockn updated this revision to Diff 2081.Jun 17 2019, 4:19 PM

self review

natekupp accepted this revision.Jun 17 2019, 4:29 PM
This revision was automatically updated to reflect the committed changes.