fix(sdk): honor KINDLING_ARTIFACTS_STORAGE_PATH in DatabricksAPI job submission - #226
Open
jtdossett wants to merge 1 commit into
Open
fix(sdk): honor KINDLING_ARTIFACTS_STORAGE_PATH in DatabricksAPI job submission#226jtdossett wants to merge 1 commit into
jtdossett wants to merge 1 commit into
Conversation
…submission DatabricksAPI.from_env() only read the legacy AZURE_STORAGE_ACCOUNT/ AZURE_CONTAINER/AZURE_BASE_PATH triple, so app run/app job register failed job submission with an invalid schemeless python_file (artifacts/scripts/kindling_bootstrap.py) on any workspace configured per the gh#211/gh#207 pattern with only KINDLING_ARTIFACTS_STORAGE_PATH set (e.g. a Databricks Volumes path). DatabricksAPI now accepts artifacts_path, populated from KINDLING_ARTIFACTS_STORAGE_PATH in from_env, and consulted by _resolve_artifacts_storage_path ahead of the legacy storage_account/ container branch. _resolve_python_file needed no change - it already does a scheme-agnostic join. Existing AZURE_STORAGE_ACCOUNT-only configurations are unaffected (self.artifacts_path stays None, so the untouched legacy branch fires exactly as before). When neither is configured, artifact resolution now raises an actionable ValueError instead of silently returning the bare "artifacts" literal that caused this bug. Fixes gh#216
There was a problem hiding this comment.
Pull request overview
Updates the Kindling SDK Databricks job-submission path to correctly honor KINDLING_ARTIFACTS_STORAGE_PATH (notably /Volumes/... roots), preventing invalid relative python_file references during kindling app run / job registration.
Changes:
- Added
artifacts_pathtoDatabricksAPIand populate it fromKINDLING_ARTIFACTS_STORAGE_PATHinfrom_env(). - Updated
_resolve_artifacts_storage_path()to preferartifacts_pathand to raise an actionableValueErrorinstead of falling back to a bare"artifacts"literal. - Added unit tests covering env precedence, Volumes path behavior, and the new failure mode; documented the fix in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/kindling_sdk/kindling_sdk/platform_databricks.py |
Adds artifacts_path support and improves artifacts-path resolution/error behavior for Databricks job submission. |
tests/unit/test_platform_databricks_sdk_job_config.py |
Adds unit coverage for Volumes-only configuration and the new ValueError behavior. |
CHANGELOG.md |
Records the Databricks SDK fix under Unreleased. |
Comment on lines
+218
to
+223
| raise ValueError( | ||
| "Databricks artifacts location is not configured. Set " | ||
| "KINDLING_ARTIFACTS_STORAGE_PATH (e.g. /Volumes/<catalog>/<schema>/" | ||
| "<volume>/kindling or abfss://...) or the legacy AZURE_STORAGE_ACCOUNT " | ||
| "(+ optional AZURE_CONTAINER, AZURE_BASE_PATH)." | ||
| ) |
Collaborator
Author
There was a problem hiding this comment.
Fair point, but keeping this out of scope for this fix: the ValueError here only fires when nothing is configured for either mode, and the classic-mode env vars (KINDLING_DATABRICKS_CLASSIC_ARTIFACTS_PATH/KINDLING_DATABRICKS_CLASSIC_BOOTSTRAP_ROOT) already have their own resolution path elsewhere in this method — this issue targeted the UC/Volumes gap specifically. Enumerating every mode's env vars in one shared error message is a reasonable follow-up but not a correctness issue introduced by this change.
This file contains hidden or 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
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
DatabricksAPIjob submission now honorsKINDLING_ARTIFACTS_STORAGE_PATHwhen resolving the artifacts storage path, instead of silently falling back to the legacyAZURE_STORAGE_ACCOUNT/container path (or raising an opaque error).ValueErrorinstead of returning a bare"artifacts"fallback.Test plan
poe test-unit— 2344 passed, 2 skippedtests/unit/test_platform_databricks_sdk_job_config.pycoverfrom_envprecedence, missing-attribute guard,ValueErrorraise, and the/Volumes/...path caseCloses #216
🤖 Generated with Claude Code