[dbt] utility class for managing artifacts

[alangenfeld]

The goal of this class is to centralize the necessary complexity that comes from seeking to provide both a nice developer experience and a performant deployed experience.

Currently, that complexity surfaces in different places resulting in a rough experience

1. Surfacing this code as part of the scaffold and docs

import os
from pathlib import Path

from dagster_dbt import DbtCliResource

dbt_project_dir = Path(__file__).joinpath("..", "..", "..").resolve()
dbt = DbtCliResource(project_dir=os.fspath(dbt_project_dir))

# If DAGSTER_DBT_PARSE_PROJECT_ON_LOAD is set, a manifest will be created at run time.
# Otherwise, we expect a manifest to be present in the project's target directory.
if os.getenv("DAGSTER_DBT_PARSE_PROJECT_ON_LOAD"):
    dbt_manifest_path = (
        dbt.cli(
            ["--quiet", "parse"],
            target_path=Path("target"),
        )
        .wait()
        .target_path.joinpath("manifest.json")
    )
else:
    dbt_manifest_path = dbt_project_dir.joinpath("target", "manifest.json")

and instructing users set an env var any time they run dagster dev via DAGSTER_DBT_PARSE_PROJECT_ON_LOAD=1 dagster dev

2. Handling the use of a package data directory, notably for pex packaging used by Dagster Cloud Serverless. Currently, the scaffold we present in that condition fails to load locally due to mishandling the assumptions made in different places.

3. The CI/CD step that has to does the preparation that the code assumes, such as https://github.com/dagster-io/dagster-cloud-action/blob/7c2f65f979a21da1e621f768af6db5265586ee91/github/serverless/dbt/deploy.yml#L47-L56

A before and after of what this would look like for users can be seen in the in the upstack PR https://github.com/dagster-io/dagster/pull/19925/files

How I Tested These Changes

added some unit tests, additional coverage from existing tests in upstack PR that converts scaffold to use this class

[alangenfeld]

• [dbt] use artifact utility class in scaffold #19925
• [dbt] utility class for managing artifacts #19923 👈
• dagster dev env var #19748
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @alangenfeld and the rest of your teammates on Graphite

[github-actions]

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-qdxd0wrv7-elementl.vercel.app
https://al-02-20--dbt-utility-class-for-managing-artifacts.dagster.dagster-docs.io

Direct link to changed pages:

• https://dagster-docs-qdxd0wrv7-elementl.vercel.app
  https://al-02-20--dbt-utility-class-for-managing-artifacts.dagster.dagster-docs.io/tutorial/saving-your-data

[github-actions]

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-pq317c25t-elementl.vercel.app
https://al-02-20--dbt-utility-class-for-managing-artifacts.core-storybook.dagster-docs.io

Built with commit 7d6ba17.
This pull request is being automatically deployed with vercel-action

[alangenfeld]

still needs polish but i think this is ready for feedback, https://github.com/dagster-io/dagster/pull/19925/files is a good place to check how this is expected to play out

[rexledesma]

Exciting

[rexledesma]

I can imagine that this is an abstractmethod for overriding this class in the future

[alangenfeld]

Thinking was that prepare_command would avoid a good chunk of cases. Do you think _ prefix should get dropped if people might override?

[schrockn]

Have some questions and suggestions

[schrockn]

what if someone uses dagster dev against pre-compiled manifest.json?

[alangenfeld]

The behavior from the existing recommended setup is that if the env var is set we will still generate a new one, so thats what i went with.

I think if you are actively iterating locally, the previous manifest.json will be there, so its hard to distinguish pre-compiled from development iteration.

I think if you want to develop against a precompiled the output would be to not use this class at all.

I could be convinced that an opt-out env var makes sense though.

[schrockn]

If this is meant to be used directly by users, an example usage in the PR description with before/after would be useful.

handled inlines, summary update with link to PR for before & after

[OwenKephart]

Might just be my brain breaking, but I found the internal naming conventions here pretty difficult to follow, and so suggested some updates / small refactors

Let me know if I'm off base on my understanding of things here

High level though, I'm excited about this behavior and think this is a good interface for people to interact with.

[OwenKephart]

I think there was more discussion elsewhere, but any particular reason for not logging at all instead of just logging at DEBUG level vs. INFO level? I feel like that's a more common pattern in our codebase.

No strong feelings there just trying to understand.

[OwenKephart]

should this get **kwargs passed in as well?

[OwenKephart]

Related to my larger comment at the top but I'm also finding this confusing as to why _should_prepare_at_runtime is related to what path this should be pointing at. Functionally, I get that the method checks if you're currently running from a dagster dev invocation, but that seems like a separate concern from if DAGSTER_DBT_PARSE_PROJECT_ON_LOAD is set or not. I get that it'd be fairly unlikely to set that outside of a development context, but if we want to make that assertion we should just call this method "_is_local_environment" or something.

That'd make other code paths clearer as well (took me a bit to wrap my head around _should_prepare_at_runtime = this code will only run when in a dev environment)

[OwenKephart]

For some reason this path shuffling stuff was extremely hard to wrap my head around, and I feel like the weirdness here is potentially stemming from the naming here.

Is it correct that you'd only want to set a package_data_dir in the case that your project_dir is something that is in a different directory than your dagster code? My feeling is that this should be called deployed_project_dir or something of the sort, and the current-day project_dir parameter can be called local_project_dir (at least internally, the user-facing parameter can be the same)

Then, there could be a single property current_project_dir which returns either self._local_project_dir or self._deployed_project_dir depending on the context (i.e. if DAGSTER_IS_DEV_CLI is not set and self._deployed_project_dir is non-None, return deployed dir, otherwise return current dir)

This is almost, but not quite, what self._base_dir is, but I feel like that property muddies the waters a bit and could be removed entirely, as it's only used in get_cli_resource (which could become trivial with that property) and setting the manifest path (which could also use the current_project_dir, as far as I can tell)

[alangenfeld]

thanks for the review @OwenKephart , did a rev incorporating your feedback. Let me know what you think.

[OwenKephart]

Awesome 🙌

Only remaining thought is on adding functionality to this class -- calling it DbtArtifacts makes me think that I can do something like:

my_artifacts = DbtArtifacts(...)

@dbt_assets(manifest=my_artifacts.manifest)
def foo():
    ...

rather than manually reading from my_artifacts.manifest_path.

[alangenfeld]

rather than manually reading from my_artifacts.manifest_path

I don't think there is any reason we couldn't add something like that, especially if we make sure to use the same cached manifest load as the rest of dagster-dbt

[rexledesma]

Approving, but I think we should table get_cli_resource to a different PR until we see usage somewhere downstream.

[rexledesma]

Would like to see how this is used in an example before introducing it.

Are we expecting the following? Feels weird to force the user to instantiate DbtCliResource outside of its class.

defs = Definitions(
    ...,
    resources={
        "dbt": my_artifacts.get_cli_resource(...)
    },
)

Would prefer something else like the following, so DbtCliResource is explicit, and we continue to have typing on DbtCliResource instantiation.

defs = Definitions(
    ...,
    resources={
        "dbt": DbtCliResource(artifacts=my_artifacts, **kwargs)
    },
)

[alangenfeld]

Would like to see how this is used in an example before introducing it.

https://github.com/dagster-io/dagster/pull/19925/files is what ive been working with

Agree with your take, will pull for now and put up a PR for artifacts on DbtCliResource

[slopp]

@alangenfeld do we have WIP docs for this?

[alangenfeld]

just the API docs / doc strings at this moment

[sryza]

Generally very supportive of this direction of removing this boilerplate for users, but a concern I have about this PR is that introduces this term "prepare" which I don't think users will have many associations with in this context, and I worry they thus might find kind of confusing and opaque? Reading the PR, it took me a while to wrap my head around what it meant. Did you consider something more explicit like generate_manifest_file?

Sorry for coming in late on this.

[sryza]

One more thought on this:

• In dbt, the term "artifacts" seems to have this specific meaning: https://docs.getdbt.com/reference/artifacts/dbt-artifacts
• If I saw a class named DbtArtifacts, I would expect that it basically corresponds exactly to that, e.g. directly wraps an artifacts directory.
• Not sure about this, but might a more accurate name be DbtProject?

[alangenfeld]

Introduces this term "prepare" which I don't think users will have many associations with in this context, and I worry they thus might find kind of confusing and opaque? Reading the PR, it took me a while to wrap my head around what it meant. Did you consider something more explicit like generate_manifest_file?

In addition to generating the manifest, this method optionally takes care of

• copying over the project in to a package data directory (useful for dagster cloud serverless PEX users)
• [future PR] taking care of uploading a manifest.json during prod deployment and downloading it in other (branch) deployments to facilitate --defer

This led me to use a more encompassing term like prepare . I am not attached to that name, can try iterating on that in subsequent changes.

DbtArtifacts

Agree overlapping but not being aligned with dbt's definition is not great. I could see DbtProject working. Will also factor this in to coming changes.