New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[docs] [pipes] - Add Databricks integration guide #17114

Merged

smackesey merged 12 commits into master from erin/pipes-databricks

Nov 7, 2023

Contributor

erinkcochran87 commented Oct 10, 2023 •

edited

Loading

Summary & Motivation

This PR adds a guide for integrating Dagster Pipes with Databricks.

TODO/?s:

Finish descriptions of SubmitTask spec
Finish UI section
Check in code examples
Check on some of the PyObjects - may be out of sync due to changes in libraries
Add info about sending data back to Dagster (Step 2)

How I Tested These Changes

👀 , bk


          First pass

1f2eb3a

erinkcochran87 requested review from smackesey and yuhan

October 10, 2023 12:55

Contributor Author

erinkcochran87 commented Oct 10, 2023

@yuhan @smackesey - Here's a rough first pass of the Databricks + Pipes guide. There are still some TODOs/Questions (see PR description) that I could use help with.

Sean - do you have a working example I can use to take screenshots of the Dagster UI?


          Merge branch 'master' into erin/pipes-databricks

85ff9bf

github-actions bot commented Oct 10, 2023 •

edited

Loading

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-6lelym987-elementl.vercel.app
https://erin-pipes-databricks.dagster.dagster-docs.io

Direct link to changed pages:

smackesey reviewed

View reviewed changes

Collaborator

smackesey left a comment

Thanks for this!

A lot of comments-- there are many (and will be more) subtle things that I think you won't know very well since you don't know Databricks/Pipes the way I do, having been banging my head against them for weeks.

I think the most efficient thing will be for you to incorporate (or dispute) the comments I provided here and make whatever other changes you want to where you are semi-satisfied, and I can follow up directly with tweaks rather than further commenting. We could do this by either you merging a draft or me just taking over pushing to the PR, up to you.

docs/content/guides/dagster-pipes/integrating-databricks-with-dagster-pipes.mdx Outdated Show resolved Hide resolved

docs/content/guides/dagster-pipes/integrating-databricks-with-dagster-pipes.mdx Outdated

+              <Note>
+                <strong>Heads up!</strong> This guide focuses on using an out-of-the-box
+                Databricks resource. For further customization, use the{" "}

Collaborator

smackesey Oct 10, 2023

"Databricks resource" -> "Databricks Pipes client"

Contributor Author

erinkcochran87 Oct 10, 2023

But it (sort of?) functions like a resource, and is configured like one. Resource is a term Dagster users are already familiar with, whereas this is a new term. Do you feel strongly about changing this?

docs/content/guides/dagster-pipes/integrating-databricks-with-dagster-pipes.mdx Outdated Show resolved Hide resolved

docs/content/guides/dagster-pipes/integrating-databricks-with-dagster-pipes.mdx Outdated Show resolved Hide resolved

docs/content/guides/dagster-pipes/integrating-databricks-with-dagster-pipes.mdx Outdated Show resolved Hide resolved

docs/content/guides/dagster-pipes/integrating-databricks-with-dagster-pipes.mdx Outdated

+                   - `spark_python_task` - An object specifying the Python file the job should run, which contains the following properties:
+                     - `python_file` - The URI of the Python file to run, located in DBFS.
+                     - `source` - TODO - is this required?

Collaborator

smackesey Oct 10, 2023

I don't think we should attempt to formally document the various fields here. This is part of the Databricks API and has nothing to do with Dagster per se. We might just say something like:

Here we are targeting an existing python script on DBFS. Refer to the Databricks SDK/API documentation for more information on how to specify a Databricks task.

Contributor Author

erinkcochran87 Oct 10, 2023 •

edited

Loading

Yeah, about that - did you find good documentation that describes how to do this? What I found was pretty light and didn't totally match up here. I also would prefer not to include these fields, but if we can't point to good documentation, I don't think it's a good experience for users of our integration.

docs/content/guides/dagster-pipes/integrating-databricks-with-dagster-pipes.mdx Outdated Show resolved Hide resolved

docs/content/guides/dagster-pipes/integrating-databricks-with-dagster-pipes.mdx Outdated Show resolved Hide resolved

docs/content/guides/dagster-pipes/integrating-databricks-with-dagster-pipes.mdx Outdated Show resolved Hide resolved

docs/content/guides/dagster-pipes/integrating-databricks-with-dagster-pipes.mdx Outdated Show resolved Hide resolved

erinkcochran87 added 2 commits

October 10, 2023 15:06


          Review comments

791cb7c


          Remove line

46286d5

Contributor Author

erinkcochran87 commented Oct 10, 2023

A lot of comments-- there are many (and will be more) subtle things that I think you won't know very well since you don't know Databricks/Pipes the way I do, having been banging my head against them for weeks.

I'm sure 😂 Definitely a good learning opportunity for me here, though!

I think the most efficient thing will be for you to incorporate (or dispute) the comments I provided here and make whatever other changes you want to where you are semi-satisfied, and I can follow up directly with tweaks rather than further commenting.

That sounds good! I did respond to a few of your comments, mostly asking for clarification/reasoning on a few things. It's only a few - if you don't mind responding to those at least, I'd love to have the context for the suggestions.

We could do this by either you merging a draft or me just taking over pushing to the PR, up to you.

Feel free to push to this branch and we can work together! I think that's the easiest way to do this.

erinkcochran87 added 3 commits

October 26, 2023 20:59


          First pass

a40e33e


          Review comments

7dfaf29


          Remove line

fb94fc5

smackesey force-pushed the erin/pipes-databricks branch from 46286d5 to 3d8b0d8 Compare

October 27, 2023 14:49

Collaborator

smackesey commented Oct 27, 2023

Current dependencies on/for this PR:

master
- PR [docs] [pipes] - Add Databricks integration guide #17114 👈

This comment was auto-generated by Graphite.

github-actions bot commented Oct 27, 2023

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-5n2aagbry-elementl.vercel.app
https://erin-pipes-databricks.components-storybook.dagster-docs.io

Built with commit 3d8b0d8.
This pull request is being automatically deployed with vercel-action

github-actions bot commented Oct 27, 2023

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-4yo7866qt-elementl.vercel.app
https://erin-pipes-databricks.core-storybook.dagster-docs.io

Built with commit 3d8b0d8.
This pull request is being automatically deployed with vercel-action

github-actions bot commented Oct 27, 2023

Deploy preview for dagster-university ready!

✅ Preview
https://dagster-university-dn634922c-elementl.vercel.app
https://erin-pipes-databricks.dagster-university.dagster-docs.io

Built with commit 3d8b0d8.
This pull request is being automatically deployed with vercel-action

smackesey force-pushed the erin/pipes-databricks branch 4 times, most recently from 72e608e to 211f481 Compare

October 27, 2023 15:53


          new commit

1eb547f

smackesey force-pushed the erin/pipes-databricks branch from 211f481 to 1eb547f Compare

October 27, 2023 16:00

Collaborator

smackesey commented Oct 27, 2023

@erinkcochran87 @yuhan OK this has been updated. You'll want to do a final proofread or perhaps massage it for other reasons but I think it's in good shape:

I made the steps less granular. This is a stylistic choice but made it much easier to write the code snippets and ensure their correctness. Also FWIW I personally find large code blocks that are "complete" easier to comprehend, and I'm pretty sure any user interested in this guide is going to be highly technical.
the snippet code now has tests, although those tests are set up to skip on BK due to the difficulty of running Databricks jobs on BK. However, they can be run locally by:

export DATABRICKS_HOST=<host>
export DATABRICKS_TOKEN=<your personal access token>
pytest examples/docs_snippets/docs_snippets_tests/guides_tests/dagster_pipes_tests/test_databricks.py::test_databricks_asset

Added screenshots.
Filled in TODOs.
Various other tweaks.
One caveat is that the final "Advanced" section is not under test (though it is linted and type-checked, which is an improvement), in part due to the way it's written. I think that is OK for now.
Deleted the Pipes content from Databricks README since it now lives here.

Collaborator

smackesey commented Nov 6, 2023

@erinkcochran87 Have you looked at this yet/can we get it merged? There is a user I'd like to share it with.

erinkcochran87 added 4 commits

November 6, 2023 16:07


          Merge branch 'erin/pipes-databricks' of https://github.com/dagster-io…

45d312e

…/dagster into erin/pipes-databricks


          Updates

30acbe9


          Update sidenav

0d08f96


          Fix typo

9cfbea9

smackesey approved these changes

View reviewed changes

Collaborator

smackesey left a comment

great

smackesey merged commit b2ca074 into master

1 check passed

smackesey deleted the erin/pipes-databricks branch

November 7, 2023 13:35

smackesey added a commit that referenced this pull request


          [docs] [pipes] - Add Databricks integration guide (#17114)

9e08133

## Summary & Motivation

This PR adds a guide for integrating Dagster Pipes with Databricks.

TODO/?s:

- [x] Finish descriptions of `SubmitTask` spec
- [x] Finish UI section
- [x] Check in code examples
- [x] Check on some of the `PyObjects` - may be out of sync due to
changes in libraries
- [x] Add info about sending data back to Dagster (Step 2)

## How I Tested These Changes

eyes, bk

---------

Co-authored-by: Sean Mackesey <[email protected]>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet