-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Feature/databricks oauth #16155
Feature/databricks oauth #16155
Conversation
@rexledesma are you able to take a look at this one? |
@smackesey Could you take a look? Some bugs were uncovered that look like fallout from #14673 |
I'll take a look |
@smackesey / @rexledesma - just tested the latest dagster / dagster-databricks release and it did in fact break the step launcher. Attempting to run a job with the step launcher configured gives this error:
Would you like me to cherry-pick my fixes into another PR? |
## Summary & Motivation [This PR (#14673)](#14673) broke the Databricks step launcher due to an incomplete implementation of the new databricks-sdk API. I cherry-picked my fixes from [another PR](#16155) that seems like it may have been lost in the ether. ## How I Tested These Changes Spun up a new environment with dagster / dagster-databricks 1.4.11 and launched a job with an op that uses the `databricks_pyspark_step_launcher` --------- Co-authored-by: Sean Mackesey <[email protected]>
## Summary & Motivation [This PR (dagster-io#14673)](dagster-io#14673) broke the Databricks step launcher due to an incomplete implementation of the new databricks-sdk API. I cherry-picked my fixes from [another PR](dagster-io#16155) that seems like it may have been lost in the ether. ## How I Tested These Changes Spun up a new environment with dagster / dagster-databricks 1.4.11 and launched a job with an op that uses the `databricks_pyspark_step_launcher` --------- Co-authored-by: Sean Mackesey <[email protected]>
@zyd14 looks like you merged instead of rebased? If you're having git problems or would otherwise prefer I can just take this over to move it over the line. |
@smackesey Ah okay, honestly I'm not as familiar with rebase strategies. I thought I clicked through rebasing from master in my IDE but must've messed something up along the way. If you don't mind taking over that's totally fine with me. |
e7a2cf8
to
9d841fc
Compare
…to the DatabricksClient. Throwing an exception if the `api_client` or `client` properties are access when oauth credentials are used, as those clients do not support oauth logins.
2217f58
to
57d20f8
Compare
## Summary & Motivation Fixes dagster-io#16126 about enabling oauth authentication for Databricks service principals ## How I Tested These Changes Unit tests + manually tested that Databricks steps can be launched via the step launcher using either a personal access token or oauth client ID + client secret. During this manual testing I also uncovered a few bugs around the `submit()` call to the `databricks-sdk` which were preventing jobs from being launched (databricks-sdk requires a [task_key parameter](dagster-io@7d2466c), and reading a file that doesn't exist now raises a [DatabricksError](dagster-io@fd91538)). Currently the step launcher is broken as-is and will not work without these changes, so if this whole PR doesn't get merged before the next release then those two commits should probably at least get cherry-picked into a different PR and merged in. I should also note that it appears that the most recent release 0.20.10 works, and that the breaking changes must have been made since the last release. --------- Co-authored-by: Sean Mackesey <[email protected]>
Summary & Motivation
Fixes #16126 about enabling oauth authentication for Databricks service principals
How I Tested These Changes
Unit tests + manually tested that Databricks steps can be launched via the step launcher using either a personal access token or oauth client ID + client secret.
During this manual testing I also uncovered a few bugs around the
submit()
call to thedatabricks-sdk
which were preventing jobs from being launched (databricks-sdk requires a task_key parameter, and reading a file that doesn't exist now raises a DatabricksError). Currently the step launcher is broken as-is and will not work without these changes, so if this whole PR doesn't get merged before the next release then those two commits should probably at least get cherry-picked into a different PR and merged in. I should also note that it appears that the most recent release 0.20.10 works, and that the breaking changes must have been made since the last release.