Add script and update `build-and-test-dbt` workflow to push dbt Python dependencies to S3 #435

jeancochrane · 2024-05-08T15:28:56Z

This PR adds a new script deploy_dbt_model_dependencies.sh that downloads dependencies for dbt Python models, bundles them into zip archives, and pushes them to S3. It also updates the build-and-test-dbt workflow to use the new script, including the following features:

If a dbt cache exists, only deploy packages for models that are new or have changed
If a caller has dispatched the workflow for a specific set of models, only deploy packages for those models
In all cases, only deploy package bundles that are new or have changed
Deploy a separate package bundle for each model
Deploy package bundles to separate S3 locations for dev, CI, and prod, and isolate dev/CI environments by user or branch

Note that this PR leaves out of scope the additional step to refactor the existing reporting.ratio_stats model to use this new dependency management system. That step will be handled in a follow-up issue (#439).

Closes #417.

Testing

Evidence of build-and-test-dbt pushing packages for changed models to S3 on synchronize event: Link

Evidence of build-and-test-dbt skipping dependencies that have not changed on synchronize event: Link

Evidence of build-and-test-dbt skipping models that have no Python dependencies on workflow_dispatch event: Link

Evidence of cleanup-dbt-resources removing packages from S3 on closed event: Link

…w for testing

… steps if requirements were found

…rkflow

…urposes of testing deployment

…t-dependencies

…ore installing

…f models

…rget

…ferent CI branches

…thon-dependencies-to-s3

… build-and-test-dbt

…y_dbt_model_dependencies.sh

…pendencies in the workflow

jeancochrane · 2024-05-09T18:00:36Z

Closing this to test the cleanup step.

…ncies.sh

.github/scripts/cleanup_dbt_resources.sh

jeancochrane · 2024-05-09T21:49:54Z

.github/workflows/build_and_test_dbt.yaml

+                dbt list \
+                  --quiet -t "$TARGET" -s state:modified state:new \
+                  --state "$STATE_DIR" --resource-types model \
+                  --output json --output-keys name | \


This modified/new state selection logic is shared in a couple of places. Might be worth a small follow-up issue to factor them all out into a shared YAML selector.

Absolutely, let's give it to one of the juniors. Can you write up a brief issue?

Done, see #446! Note that I expect a junior would need some detailed guidance for this one, since the issue requires mastery of node selection syntax and YAML anchors in order to get the design right.

.github/workflows/build_and_test_dbt.yaml

dbt/macros/get_s3_dependency_dir.sql

jeancochrane · 2024-05-09T22:06:43Z

dbt/macros/get_s3_dependency_dir.sql

+-- Get the S3 location that is used to store Python model dependencies
+-- for the current target


Ideally we would just add a new attribute s3_dependency_dir to our profiles defined in profiles.yml, and then we could have full control over the location where dependencies get stored for each target. That doesn't appear to be possible without extending the dbt-athena plugin itself to support a new profile variable, so this macro represents a workaround that allows us to compute a dependency dir based on the s3_data_dir variables set in our profiles.

I think we'll probably need a Python version of this macro as well, but I'm leaving that for myself to figure out later when I tackle #439.

jeancochrane · 2024-05-09T22:11:25Z

dbt/macros/get_s3_dependency_dir.sql

+        {% endif %}
+        {% set dir_suffix = "/" ~ head_ref %}
+    {% endif %}
+    {% set s3_dependency_dir = target.s3_data_dir ~ "packages" ~ dir_suffix %}


One downside of this approach is that it stores packages inside the data dir, when ideally they would be stored alongside the data dir. For clarity, here are the three paths for the three different targets we support:

dev

data dir: s3://ccao-dbt-athena-dev-us-east-1/data/

dependency dir: s3://ccao-dbt-athena-dev-us-east-1/data/packages/<username>/

ci

data dir: s3://ccao-dbt-athena-ci-us-east-1/data/

dependency dir: s3://ccao-dbt-athena-ci-us-east-1/data/packages/<branch_slug>/

prod

data dir: s3://ccao-athena-ctas-us-east-1/

dependency dir: s3://ccao-athena-ctas-us-east-1/packages/

I think prod is probably the most reasonable of the three. They all feel acceptable right now, but not perfect. What do you think?

suggestion (non-blocking): Well, if we're following our existing schema for the dev, ci, prod split, something like this might be better:

dev

data dir: s3://ccao-dbt-athena-dev-us-east-1/data/

dependency dir: s3://ccao-dbt-athena-dev-us-east-1/packages/<username>/

ci

data dir: s3://ccao-dbt-athena-ci-us-east-1/data/

dependency dir: s3://ccao-dbt-athena-ci-us-east-1/packages/<branch_slug>/

prod

data dir: s3://ccao-athena-ctas-us-east-1/

dependency dir: s3://ccao-athena-packages-us-east-1/

This gives us tighter control over permissions and reduces the blast radius of accidental removal of prod stuff.

@dfsnow I agree, that pattern would be cleaner, but per my comment above we aren't actually able to configure the dependency directory as a dedicated profile variable, so it needs to be derivable from other variables like s3_data_dir. This is why I proposed the organization sketched out in this PR, as imperfect as it is; if you have suggestions for a better one that is derivable from existing profile variables I would be grateful!

Edited to add: I suppose we could implement a scheme that is independent of profile variables by just hardcoding the pattern into the get_s3_dependency_dir macro based on the target, but it would make me a bit nervous to decouple this particular S3 path config from profiles.yml when the rest of our S3 path configs are stored there. Still, it's one option for working around the limitations we're facing.

Oh oh, I gotcha, sorry I didn't read closer. What about adding some env vars to dbt_project.yml:

vars: s3_package_dir_dev: s3://ccao-dbt-athena-dev-us-east-1/packages/ s3_package_dir_ci: s3://ccao-dbt-athena-ci-us-east-1/packages/ s3_package_dir_prod: s3://ccao-athena-packages-us-east-1/

And then conditionally selecting the directory based on the suffix of the env var. Feels pretty hacky, but at least then the vars are declared and not stored in the conditional logic itself.

Otherwise, I think your schema is fine! All of this should be relatively easy to change if we ever want/need to move package locations.

I like that idea! Done in 02cc83a. Note that I've been using dependency instead of package to name these resources since I don't want them to be confused with dbt packages, but I don't feel super strongly about it, particularly since the name of the config attribute is packages anyway.

.github/workflows/build_and_test_dbt.yaml

dbt/macros/tests/test_generate_schema_name.sql

dfsnow

This is awesome @jeancochrane. Just a minor issue and a suggestion and I think this is set to go.

dfsnow · 2024-05-13T18:35:30Z

dbt/macros/get_s3_dependency_dir.sql

+        {% endif %}
+        {% set dir_suffix = "/" ~ head_ref %}
+    {% endif %}
+    {% set s3_dependency_dir = target.s3_data_dir ~ "packages" ~ dir_suffix %}


suggestion (non-blocking): Well, if we're following our existing schema for the dev, ci, prod split, something like this might be better:

dev

data dir: s3://ccao-dbt-athena-dev-us-east-1/data/

dependency dir: s3://ccao-dbt-athena-dev-us-east-1/packages/<username>/

ci

data dir: s3://ccao-dbt-athena-ci-us-east-1/data/

dependency dir: s3://ccao-dbt-athena-ci-us-east-1/packages/<branch_slug>/

prod

data dir: s3://ccao-athena-ctas-us-east-1/

dependency dir: s3://ccao-athena-packages-us-east-1/

This gives us tighter control over permissions and reduces the blast radius of accidental removal of prod stuff.

dfsnow · 2024-05-13T18:43:50Z

.github/scripts/cleanup_dbt_resources.sh

+)
+
+echo "Deleting Python model requirements at $s3_dependency_dir"
+aws s3 rm "$s3_dependency_dir" --recursive


issue (non-blocking): Recursive removal makes me nervous. Let's make sure the dbt Athena role is very tightly scoped to only allow removals of the bare minimum. Can be addressed in a follow up PR to the infra repo.

This is a bit of a tricky issue: Unfortunately, dbt-athena requires DeleteObject permissions over all objects in s3_data_dir (docs). It's possible these docs are misleading and there's a way to scope the permissions more tightly, but I think the only way to figure it out would be through trial and error.

One alternative would be to remove this step from cleanup_dbt_resources.sh and rely on the existing S3 management policy that deletes everything in the CI data/ directory after 30 days. This wouldn't be perfect from a cost management perspective, but it might be good enough. It would also have the negative side effect of making it so that Python model dependencies don't get properly cleaned up when a user runs this script against the dev target, but we could also handle that by gating this block behind a $TARGET == 'dev' conditional block.

I'm open to any of these options -- curious what you think!

Hmmm alright, as long as there's no way for that call to ever target anything other than the target env buckets then I think it's okay. I can dig a little more on this when we do our permissions cleanup.

Sounds good! I think we're also slightly more protected by the switch to variables for defining s3_dependency_dir in 02cc83a, since now our S3 paths for dependencies don't overlap with our paths for data.

dbt/macros/get_s3_dependency_dir.sql

dfsnow · 2024-05-13T18:52:42Z

.github/workflows/build_and_test_dbt.yaml

+                dbt list \
+                  --quiet -t "$TARGET" -s state:modified state:new \
+                  --state "$STATE_DIR" --resource-types model \
+                  --output json --output-keys name | \


Absolutely, let's give it to one of the juniors. Can you write up a brief issue?

.github/scripts/deploy_dbt_model_dependencies.sh

…thon-dependencies-to-s3

…sting

jeancochrane · 2024-05-14T17:16:40Z

~~@dfsnow Want to give this one more look before I merge?~~ Never mind, needs one more change!

dbt/models/default/schema/default.vw_pin_address.yml

…s for testing" This reverts commit 96954cd.

…n_address for testing"" This reverts commit a5f5262.

…lt.vw_pin_address for testing""" This reverts commit 05460ba.

jeancochrane and others added 2 commits May 7, 2024 22:15

Sketch out deploy-dbt-requirements workflow

c6a8651

Temporarily add pull_request event to deploy-dbt-requirements workflo…

946abe7

…w for testing

jeancochrane linked an issue May 8, 2024 that may be closed by this pull request

Add workflow to deploy Python dependencies to S3 #417

Closed

jeancochrane force-pushed the jeancochrane/417-add-workflow-to-deploy-python-dependencies-to-s3 branch from 2650718 to 074685c Compare May 8, 2024 16:03

Update deploy-dbt-requirements workflow to only run bundle + upload…

09cc186

… steps if requirements were found

jeancochrane force-pushed the jeancochrane/417-add-workflow-to-deploy-python-dependencies-to-s3 branch from 074685c to 09cc186 Compare May 8, 2024 16:07

jeancochrane changed the title ~~Add deploy-dbt-requirements workflow for pushing dbt Python requirements to S3~~ Add deploy-dbt-dependencies workflow for pushing dbt Python dependencies to S3 May 8, 2024

Rename "requirements" -> "dependencies" in deploy-dbt-dependencies wo…

e6b44ea

…rkflow

jeancochrane force-pushed the jeancochrane/417-add-workflow-to-deploy-python-dependencies-to-s3 branch from ae19253 to e6b44ea Compare May 8, 2024 16:16

jeancochrane and others added 10 commits May 8, 2024 11:20

Add some dummy dependencies to reporting.res_report_summary for the p…

2a88853

…urposes of testing deployment

Use process substitution to iterate packages in deploy-dbt-dependencies

98e775a

Switch S3 bucket location to CI for the purposes of testing deploy-db…

1f57c9a

…t-dependencies

Factor deploy_dbt_dependencies out into a script and diff changes bef…

1998083

…ore installing

Rename deploy_dbt_dependencies.sh and allow callers to pass in list o…

0e610b9

…f models

Set remote location for Python model dependencies depending on dbt ta…

ac8862e

…rget

Update get_s3_dependency_dir so that the dirname is different for dif…

0887b5b

…ferent CI branches

Adjust cleanup_dbt_resources.sh to rm s3 dependencies

3e76ef0

Appease shellcheck

a082d16

Merge branch 'master' into jeancochrane/417-add-workflow-to-deploy-py…

13543e0

…thon-dependencies-to-s3

jeancochrane force-pushed the jeancochrane/417-add-workflow-to-deploy-python-dependencies-to-s3 branch 2 times, most recently from 49e1231 to 82fd339 Compare May 9, 2024 17:35

Move deploy_dbt_model_dependencies logic from dedicated workflow into…

960d2c3

… build-and-test-dbt

jeancochrane force-pushed the jeancochrane/417-add-workflow-to-deploy-python-dependencies-to-s3 branch from 82fd339 to 960d2c3 Compare May 9, 2024 17:37

jeancochrane added 4 commits May 9, 2024 17:45

Switch test packages model to default.vw_pin_address for ease of testing

b870b75

Make sure to append a trailing slash after s3_dependency_dir in deplo…

8cfd0ec

…y_dbt_model_dependencies.sh

Empty commit to test state modificiation for reqs deployment script

5124115

Disable shellcheck double quote rule when calling deploy_dbt_model_de…

a41e5ca

…pendencies in the workflow

jeancochrane closed this May 9, 2024

Remove testing packages for default.vw_pin_address

86b55d5

jeancochrane reopened this May 9, 2024

jeancochrane and others added 2 commits May 9, 2024 18:07

Resolve unbound variable issue in deploy_dbt_model_dependencies.sh

9ebce20

Properly handle no modified/new resources in deploy_dbt_model_depende…

ef7cde1

…ncies.sh

jeancochrane changed the title ~~Add deploy-dbt-dependencies workflow for pushing dbt Python dependencies to S3~~ Add script and update build-and-test-dbt workflow to push dbt Python dependencies to S3 May 9, 2024

Clean up comments and docstring in deploy_dbt_model_dependencies.sh

4c970a3

jeancochrane commented May 9, 2024

View reviewed changes

jeancochrane marked this pull request as ready for review May 9, 2024 22:17

jeancochrane requested a review from a team as a code owner May 9, 2024 22:17

jeancochrane requested a review from dfsnow May 9, 2024 22:18

dfsnow approved these changes May 13, 2024

View reviewed changes

jeancochrane and others added 4 commits May 13, 2024 21:27

Merge branch 'master' into jeancochrane/417-add-workflow-to-deploy-py…

2f95bc8

…thon-dependencies-to-s3

Better handling for spaces in get_s3_dependency_dir macro

9dde41d

Move s3_dependency_dir definition to dbt_project.yml from profiles.yml

02cc83a

Temporarily add config.packages attr to default.vw_pin_address for te…

96954cd

…sting

jeancochrane requested review from dfsnow and removed request for dfsnow May 14, 2024 17:16

dfsnow self-requested a review May 14, 2024 17:31

dfsnow approved these changes May 14, 2024

View reviewed changes

dbt/models/default/schema/default.vw_pin_address.yml Outdated Show resolved Hide resolved

jeancochrane added 4 commits May 14, 2024 14:25

Rename prod s3_dependency_dir from packages -> dependencies

a579c00

Revert "Temporarily add config.packages attr to default.vw_pin_addres…

a5f5262

…s for testing" This reverts commit 96954cd.

Revert "Revert "Temporarily add config.packages attr to default.vw_pi…

05460ba

…n_address for testing"" This reverts commit a5f5262.

Revert "Revert "Revert "Temporarily add config.packages attr to defau…

0a2a19d

…lt.vw_pin_address for testing""" This reverts commit 05460ba.

jeancochrane merged commit b06af57 into master May 14, 2024
11 checks passed

jeancochrane deleted the jeancochrane/417-add-workflow-to-deploy-python-dependencies-to-s3 branch May 14, 2024 22:23

jeancochrane mentioned this pull request May 15, 2024

Use new view to pull dependency bundle in reporting.ratio_stats #453

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add script and update `build-and-test-dbt` workflow to push dbt Python dependencies to S3 #435

Add script and update `build-and-test-dbt` workflow to push dbt Python dependencies to S3 #435

jeancochrane commented May 8, 2024 •

edited

Loading

jeancochrane commented May 9, 2024

jeancochrane May 9, 2024

dfsnow May 13, 2024

jeancochrane May 13, 2024

jeancochrane May 9, 2024

jeancochrane May 9, 2024

dfsnow May 13, 2024

jeancochrane May 13, 2024 •

edited

Loading

dfsnow May 14, 2024

jeancochrane May 14, 2024 •

edited

Loading

dfsnow left a comment

dfsnow May 13, 2024

dfsnow May 13, 2024

jeancochrane May 13, 2024

dfsnow May 14, 2024

jeancochrane May 14, 2024

dfsnow May 13, 2024

jeancochrane commented May 14, 2024 •

edited

Loading

		-- Get the S3 location that is used to store Python model dependencies
		-- for the current target

Add script and update build-and-test-dbt workflow to push dbt Python dependencies to S3 #435

Add script and update build-and-test-dbt workflow to push dbt Python dependencies to S3 #435

Conversation

jeancochrane commented May 8, 2024 • edited Loading

Testing

jeancochrane commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeancochrane May 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeancochrane May 14, 2024 • edited Loading

Choose a reason for hiding this comment

dfsnow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeancochrane commented May 14, 2024 • edited Loading

Add script and update `build-and-test-dbt` workflow to push dbt Python dependencies to S3 #435

Add script and update `build-and-test-dbt` workflow to push dbt Python dependencies to S3 #435

jeancochrane commented May 8, 2024 •

edited

Loading

jeancochrane May 13, 2024 •

edited

Loading

jeancochrane May 14, 2024 •

edited

Loading

jeancochrane commented May 14, 2024 •

edited

Loading