Page MenuHomePhabricator

Creates data classes for dbt run results.
ClosedPublic

Authored by bob on Thu, Oct 1, 4:30 PM.

Details

Summary

The Data Classes

  • DbtCliResult
  • DbtRpcResult
  • DbtResult
  • NodeResult
  • StepTiming

Similarities and Differences between CLI and RPC solids
Both of the classes CliRunResults and RpcRunResults inherit the class RunResults, where most of the metadata is contained.

RunResults includes JSON logs, duration of the dbt process, and a list of results for each model that was run. Some dbt commands (for both CLI and RPC) do not actually run anything on the models; in these cases, the list of results is empty.

CliRunResults uniquely includes the shell return code and (for dbt run and dbt test only) summary statistics of run. Some dbt CLI commands do not actually run anything on the models; in these cases, the summary statistics are None.

RpcRunResults uniquely includes the state and timestamps of the dbt process on the RPC server.

Commits

  • Merge branch 'master' into dbt--improve-cli-results
  • Adds CliRunResult and testing.
  • Adds comment for pre_exec.
  • Refactors CLI solids and types.
  • Removes use of attr from dbt RPC resources.
  • Fixes and uses PolledRunResult for RPC solids.
  • Renames PolledRunResult to RpcRunResult.
  • Renamings of classes and files.
Test Plan

buildkite ofc

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Oct 1, 5:07 PM
Harbormaster failed remote builds in B18972: Diff 23042!

Fix lint issues 2, and marks dbt optional fields.

Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Oct 1, 5:53 PM
Harbormaster failed remote builds in B18976: Diff 23046!
Harbormaster failed remote builds in B18977: Diff 23047!
Harbormaster returned this revision to the author for changes because remote builds failed.Thu, Oct 1, 6:18 PM
Harbormaster failed remote builds in B18978: Diff 23048!
bob requested review of this revision.Thu, Oct 1, 6:56 PM
max requested changes to this revision.Fri, Oct 2, 5:50 PM

must fix the TODOs

python_modules/libraries/dagster-dbt/dagster_dbt/cli/solids.py
23–25

prefer to link to a particular commit if possible

151

fixme

151–152

fixme

355

fixme

355–356

fixme

440

fixme

440–441

fixme

python_modules/libraries/dagster-dbt/dagster_dbt/cli/types.py
10–17

ok, this is a lot of boilerplate tbh (vs using a namedtuple?)

19–20

should users ever construct this?

25

should the result object include the command that was run

python_modules/libraries/dagster-dbt/dagster_dbt/rpc/types.py
8–9

do users ever construct this?

This revision now requires changes to proceed.Fri, Oct 2, 5:50 PM
bob marked an inline comment as done.
  • Improves descriptions for AssetMaterializations.
  • Adds "command" property to CliRunResult.

I think I'm a bit out of context on this stuff now so will let the others approve. Are we still trying to get dwall to use this library? It seems like some of these changes (like the RpcRunResult rename) will require a lot of changes in his codebase. This isn't a bad thing, but just wanted to call it out.

Are we still trying to get dwall to use this library? It seems like some of these changes (like the RpcRunResult rename) will require a lot of changes in his codebase. This isn't a bad thing, but just wanted to call it out.

I have been cross-testing dwall's test suite as I make changes. So far, everything looks green and the migration PR isn't too messy.

python_modules/libraries/dagster-dbt/dagster_dbt/cli/types.py
10–17

Yeah, I agree it's quite a bit of boilerplate :/

Essentially, these types are Python data objects for JSON data. I considered using namedtuple and/or dataclass, but those provide convenience at the sacrifice of familiarity and explicit access restrictions.

We definitely need comprehensive docstrings for these properties, and IMO the amount of boilerplate ends up being about the same for Python object vs namedtuple vs dataclass.

19–20

Generally, no.

These classes are exported from the library so that API docs can be autogenerated and user's can see how to access the metadata. But, I don't imagine that users will construct instances directly or with from_dict.

Uses specific commit in GitHub link.

max requested changes to this revision.Wed, Oct 7, 2:35 PM
max added inline comments.
python_modules/libraries/dagster-dbt/dagster_dbt/cli/types.py
10–17

this is tenable in isolation, but our codebase is full of the namedtuple pattern...

19–20

k, please be explicit in the docstring: "users should not construct this class directly", as elsewhere in the codebase

89

If users should not call this directly, please note that in the docstring

92

need to resolve these

python_modules/libraries/dagster-dbt/dagster_dbt/cli/utils.py
14

I think we should drop this

python_modules/libraries/dagster-dbt/dagster_dbt/types.py
12

If users are not to construct this, be explicit.

15

If this is not to be constructed by users, don't provide an example of how to construct it.

28

Don't document the constructor if this is not user-facing

88

as above

123

as above

134

fixme

198

confusing name

201

should users ever construct this?

This revision now requires changes to proceed.Wed, Oct 7, 2:35 PM
  • Tags CLI solids with "kind:dbt".
  • Rename result classes.
  • Refactors classes into named tuples.
  • Removes unused imports.

Rebased on master.

  • Fix linting issues 0.
  • Fix lint issues 1.
  • Fix lint issues 2, and marks dbt optional fields.
  • Fix lint issues 3.
  • Fix lint issues 4.
  • Improves descriptions for AssetMaterializations.
  • Adds "command" property to CliRunResult.
  • Uses specific commit in GitHub link.
  • Fix lint issues 1.
  • Tags CLI solids with "kind:dbt".
  • Rename result classes.
  • Refactors classes into named tuples.
  • Removes unused imports.
  • Removes unused imports.
bob marked 5 inline comments as done.
  • Fix linting issues 0.
  • Fix lint issues 1.
  • Fix lint issues 2, and marks dbt optional fields.
  • Fix lint issues 3.
  • Fix lint issues 4.
  • Improves descriptions for AssetMaterializations.
  • Adds "command" property to CliRunResult.
  • Uses specific commit in GitHub link.
  • Fix lint issues 1.
  • Tags CLI solids with "kind:dbt".
  • Rename result classes.
  • Refactors classes into named tuples.
  • Removes unused imports.
  • Uncomments checks.
  • Rename CLI asset materializations.
bob marked 18 inline comments as done.
  • Removes pre_exec from CLI execution.
  • Removes unused import signal.

let's merge this and proceed

This revision is now accepted and ready to land.Tue, Oct 13, 8:19 PM
  • Fix linting issues 0.
  • Fix lint issues 1.
  • Fix lint issues 2, and marks dbt optional fields.
  • Fix lint issues 3.
  • Fix lint issues 4.
  • Improves descriptions for AssetMaterializations.
  • Adds "command" property to CliRunResult.
  • Uses specific commit in GitHub link.
  • Fix lint issues 1.
  • Tags CLI solids with "kind:dbt".
  • Rename result classes.
  • Refactors classes into named tuples.
  • Removes unused imports.
  • Uncomments checks.
  • Rename CLI asset materializations.
  • Removes pre_exec from CLI execution.
  • Removes unused import signal.
This revision was automatically updated to reflect the committed changes.