Feature/databricks oauth #16155
Merged
Feature/databricks oauth #16155
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@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? |
smackesey
added a commit
that referenced
this pull request
Sep 5, 2023
## 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]>
zyd14
added a commit
to zyd14/dagster
that referenced
this pull request
Sep 5, 2023
## 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
smackesey
approved these changes
Sep 6, 2023
zyd14
added a commit
to zyd14/dagster
that referenced
this pull request
Sep 15, 2023
## 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]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-sdkwhich 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.