filer: switch workspace upload from import-file to /workspace/import - #6149
Open
Sankalp-Mittal wants to merge 21 commits into
Open
filer: switch workspace upload from import-file to /workspace/import#6149Sankalp-Mittal wants to merge 21 commits into
Sankalp-Mittal wants to merge 21 commits into
Conversation
Collaborator
Integration test reportCommit: af88c8f
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 10 slowest tests (at least 2 minutes):
|
Sankalp-Mittal
marked this pull request as ready for review
August 4, 2026 08:22
Contributor
Approval status: pending
|
Sankalp-Mittal
marked this pull request as draft
August 4, 2026 11:10
Sankalp-Mittal
marked this pull request as ready for review
August 7, 2026 12:14
Replace POST /api/2.0/workspace-files/import-file/{path} with the multipart
variant of POST /api/2.0/workspace/import (via the SDK's Workspace.Upload +
format=AUTO). The previous endpoint is deprecated; the new one has a higher
rate limit (30 vs 20 rps/workspace) and is ~1.5-2x faster for typical bundle
deployments.
Error handling is rewritten to branch on SDK sentinels (errors.Is against
ErrNotFound / ErrResourceAlreadyExists / ErrAlreadyExists /
ErrInvalidParameterValue / ErrPermissionDenied) and the AIP-193 ErrorInfo
reason WORKSPACE_OBJECT_TYPE_MISMATCH, with a message-substring fallback for
workspaces where WP-6031 has not rolled out.
The testserver now decodes the multipart /workspace/import body and mirrors the
real endpoint's format=AUTO notebook detection. A new bodyContains option on the
fault mechanism lets a test target a single file's upload, which is no longer
possible by URL since every upload shares the /workspace/import path.
Co-authored-by: Isaac
The size limits in WorkspaceFilesClient.Write were described using internal config flag names and figures measured by hand against one workspace, neither of which a reader can verify. Replace them with the documented limits and link the sources. This corrects one figure: the notebook cap was described as a flat 10 MiB, but IPYNB notebooks are documented at 100 MB. Only source-format notebooks are capped at 10 MB, so the limit depends on how format=AUTO classifies the payload. Also restore the rate limit doc link to MaxRequestsInFlight, which the comment carried when the constant was introduced in #81. The value is unchanged. Add a comment above each test in workspace_files_client_test.go stating what it covers. Co-authored-by: Isaac
The local_code_source test filtered recorded uploads by URL path, which no longer matches: /workspace/import carries the target filename in the multipart body. Filter on the body instead, the same way auto-migrate-empty-tfstate does. --del-field raw_body is dropped because the tarball is binary and the request recorder already summarizes it as a size placeholder. This test was added in #6110, after the upload migration branch was cut, so it was not covered by the earlier fixture updates. Co-authored-by: Isaac
Two error shapes stopped mapping to fileAlreadyExistsError when Write moved to sentinel matching. A 409 carrying no error_code unwraps to ErrResourceConflict only. ErrAlreadyExists and ErrResourceAlreadyExists are siblings under it, not parents, so neither matched. The workspace returns exactly this shape, and locker relies on fs.ErrExist here to report which client holds the lock. Match the parent sentinel instead. A 400 with an empty error_code matches no sentinel but ErrBadRequest, so the notebook already-exists check added in #5106 no longer fired. Restore it; that PR notes the newer message format may not have reached all workspaces, and this change has no evidence the rollout has since completed. Both shapes now have test cases; each fails against the previous logic. Co-authored-by: Isaac
The SDK's Workspace.Upload sets X-Databricks-Workspace-Id from cfg.WorkspaceID behind a bare != "" check, so it forwards the CLI-only "none" sentinel as a literal workspace ID. That sentinel is written to .databrickscfg by `auth login --skip-workspace` and names no real workspace, so the upload is misrouted. Encode the same multipart body in newImportForm and post it through apiClient.Do, which routes the header through auth.WorkspaceIDHeaders and maps the sentinel to no header at all. Field names and layout match Upload; the `language` field is omitted because it only applies to format=SOURCE. Co-authored-by: Isaac
Write no longer calls Workspace.Upload, so the mock expectations on it asserted against a call that is gone. Drive the tests through testserver and inspect the multipart body the CLI actually sends, which pins the wire format rather than the SDK call shape: path, content, format=AUTO, and overwrite present only when requested. Adds coverage for the routing header the previous commit fixes, including the "none" sentinel case. Co-authored-by: Isaac
Tighten the size-limit block to the classification rule and its two doc links, and reorganize the path-conflict list so each bullet names the branch that handles it, including the two shapes that carry no error_code. Also move multipartContentLimit above the function that reads it. Co-authored-by: Isaac
Match the notebook extension case-insensitively, so a ".R" source is detected the way the real endpoint detects it. Also require the format field to be AUTO rather than accepting an absent value. The real endpoint defaults to SOURCE, which imports every payload as a notebook, so a caller that omits the field is a bug the fake should surface. The rejection message now reports what was received. Co-authored-by: Isaac
Replace the hand-rolled sys.argv slicing with argparse. The manual parser had to special-case --body-contains before the positionals, so the flag only worked in that one position and the usage string was maintained in two places. Behavior is unchanged: same positionals, same optional error_code default, same payload. Co-authored-by: Isaac
Guards the ToLower added to detectNotebookLanguage: the test fails against the previous case-sensitive switch, where ".R" was stored as a FILE at the unstripped path. Co-authored-by: Isaac
…port The test filtered recorded uploads by URL path, which no longer matches: /workspace/import carries the target filename in the multipart body. Both assertions still show exactly two uploads (the config and the command), so the test's point -- that the requirements file is never uploaded -- is unchanged; only the recorded request shape differs. This test was added in #6166, which merged into this branch after the upload migration, so it was not covered by the earlier fixture updates. Mirrors the fix applied to ai_runtime_task/local_code_source for the same reason. Co-authored-by: Isaac
The local_code_source test asserts the uploaded size of each .air_snapshots tarball. On Windows, git checks the packaged files out with CRLF, which adds a byte per newline and shifts every recorded size, so both Windows jobs failed with 2-4 byte differences. Pin the packaged files to eol=lf, following the per-directory pattern in acceptance/experimental/air/run-submit-deps/.gitattributes. Only the files that actually land in a tarball are listed; debug.log is excluded by .gitignore and stays unpinned. The recorded sizes only became load-bearing when uploads moved to /workspace/import, which records the multipart body; the previous endpoint put the filename in the URL and never recorded the payload. Co-authored-by: Isaac
Python's text mode translates \n to \r\n on write under Windows, so editing a file through this helper replaced its LF endings with CRLF. Pinning the checked out bytes with .gitattributes does not help, because the rewrite happens after checkout: in ai_runtime_task/local_code_source only the upload recorded after the update_file.py step still reported a different size on Windows. Pass newline="" on both the read and the write so the file's existing endings survive verbatim, in either direction. Co-authored-by: Isaac
fb99c27 wrapped unmapped upload failures with "failed to upload <path>", which changed the message the upload/internal_server_error and upload/timeout tests capture. Regenerate their goldens to match. Co-authored-by: Isaac
print_requests.py --sort orders by the whole request, including the multipart body. One synced file is out.requests.txt, whose recorded body differs between runs, so its position in the list was nondeterministic — passing locally but failing in CI. Drop --sort and sort the displayed "METHOD path" lines instead, which are stable. The contains.py assertions are unaffected. Co-authored-by: Isaac
#6196 removed the Local config field and regenerated every out.test.toml snapshot, but this test was added on this branch in parallel, so its committed snapshot still carried Local = true. The post-test "no files changed" check regenerates it and fails on the diff. Regenerate to match. Co-authored-by: Isaac
The previous newline="" fix stopped Python from rewriting \n to \r\n on write, but it also made the read preserve CRLF verbatim. Script search strings are always LF, so a multi-line replacement (e.g. duplicate_yaml_merge_key) no longer matched on Windows, where databricks.yml is checked out with CRLF, and update_file.py errored with "old=... not found". Normalize CRLF to LF on read so the LF search string matches, and keep the newline="" write so the result stays LF. Net effect on Windows: the edited file is normalized to LF, which is what every downstream upload/hash assertion expects anyway. Co-authored-by: Isaac
Replace the hand-built multipart form (newImportForm + apiClient.Do) with the SDK's multipart Workspace.Upload. To keep the CLI-only "none" workspace-id sentinel from being forwarded as a literal routing id by Upload's bare cfg.WorkspaceID != "" check, normalize it to "" in NewWorkspaceFilesClient. The error mapping and multipart wire format are unchanged, so the existing Write tests still hold; add TestNewWorkspaceFilesClientNormalizesWorkspaceID to pin the sentinel normalization. Co-authored-by: Isaac
Sankalp-Mittal
force-pushed
the
sankalp-mittal/workspace-import-migration
branch
from
August 12, 2026 13:25
2a99228 to
af88c8f
Compare
| @@ -0,0 +1 @@ | |||
| Bundle file uploads now use the multipart `POST /api/2.0/workspace/import` endpoint instead of the deprecated `POST /api/2.0/workspace-files/import-file`. The new endpoint has a documented rate limit of 30 requests per second per workspace ([API rate limits](https://docs.databricks.com/aws/en/resources/limits)), higher than the limit that applied to the previous endpoint, and is ~1.5–2× faster for typical bundle deployments. | |||
Contributor
There was a problem hiding this comment.
~1.5–2× faster for typical bundle deployments
This looks awesome! Did you validate this via some tests?
| // workspaceObjectTypeMismatchReason is the AIP-193 ErrorInfo reason attached | ||
| // by /workspace/import when overwrite=true targets a path whose existing | ||
| // object's node type differs from the upload (FILE vs NOTEBOOK). | ||
| const workspaceObjectTypeMismatchReason = "WORKSPACE_OBJECT_TYPE_MISMATCH" |
Contributor
There was a problem hiding this comment.
Can be inlined. We don't need a constant for this.
| func NewWorkspaceFilesClient(w *databricks.WorkspaceClient, root string) (Filer, error) { | ||
| // Workspace.Upload/Download forward cfg.WorkspaceID behind a bare != "" check, | ||
| // so normalize the "none" sentinel to "" to keep the SDK from routing on it. | ||
| if w.Config.WorkspaceID == auth.WorkspaceIDNone { |
Contributor
There was a problem hiding this comment.
Just curious, does not normalizing it cause the SDK to error? Maybe we should flag this with the SDK folks?
| // is still needed, both verified against a live workspace on | ||
| // 2026-08-04: | ||
| // | ||
| // - Rollout lag: universe #2019174 merged 2026-06-03 with its |
Contributor
There was a problem hiding this comment.
Do we need this? 2026-06-03 was more than 2 months ago. This should be rolled out by now.
| // build still return WCS-worded collisions without details. | ||
| // That half is temporary. | ||
| // | ||
| // - Message (b) above is thrown by webapp, which #2019174 never |
Contributor
There was a problem hiding this comment.
Can we fix this in the backend? Kyle can help with this (we can send a PR or ask him to fix this). Checking the error message is always fragile and break easily if the backend changes the error message.
Contributor
There was a problem hiding this comment.
Honestly fixing this can be optional - if we can confirm that no CLI clients rely on exactly this error. We should try getting the error fixed though in the backend if it's an easy fix.
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
3 participants
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.
What
DABs uploads workspace files one file at a time. This switches the upload call in
libs/filerfrom the old, deprecated endpoint to the new one:POST /api/2.0/workspace-files/import-file/{path}POST /api/2.0/workspace/import(multipart form)Uploads stay one-file-at-a-time. Nothing else about deploy changes.
Why
The old endpoint is deprecated. The new one allows 30 uploads/sec per workspace instead of 20, has its own rate-limit budget, and scales better — so deploys hit fewer 429 (rate-limited) errors.
We do not use zip folder upload (the other option discussed): a zip upload can't overwrite a folder that already exists, and
bundle deployre-uploads into existing folders every time. So per-file on the new endpoint is the right choice.Changes
workspace_files_client.go— build the multipart request by hand instead of using the SDK'sWorkspace.Upload. The SDK helper sends the wrong workspace-routing header for the CLI's internal"none"value; building it ourselves routes correctly.format=AUTOkeeps the server deciding file-vs-notebook exactly as before.WORKSPACE_OBJECT_TYPE_MISMATCH), and fall back to matching the message text. The fallback is still needed: some workspaces don't send the structured reason yet, and one collision case comes from an older code path that never will. Both confirmed against a real workspace.Size limits (nothing gets smaller)
/workspace/importimport-fileNotebook limits are the same on both. The max file size goes up.
Testing
Unit and acceptance tests updated for the new endpoint.
Verified against a real workspace via
bundle deploy:.pyfile with no notebook header stays a file, a.ipynbbecomes a notebook, and a.lvdash.jsonbecomes a dashboard.Size limits verified against a real workspace:
exceeded max size (10485760 bytes)— the same cap the old endpoint enforced, so no regression.Not in this PR