Support tgz artifacts built from a directory or git ref #6428
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,160 @@ | ||
| package artifacts | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "io" | ||
| "os" | ||
| "os/exec" | ||
| "path" | ||
| "path/filepath" | ||
| "slices" | ||
| "strings" | ||
|
|
||
| "github.com/databricks/cli/bundle" | ||
| "github.com/databricks/cli/bundle/config" | ||
| "github.com/databricks/cli/bundle/config/mutator/aicode" | ||
| "github.com/databricks/cli/bundle/deploy/files" | ||
| "github.com/databricks/cli/libs/fileset" | ||
| libsync "github.com/databricks/cli/libs/sync" | ||
| ) | ||
|
|
||
| // buildTarballArtifact produces the gzipped tarball for a `type: tgz` artifact that | ||
| // DABs builds itself (no user `build` command). Every entry nests under a single | ||
| // top-level directory named for the artifact's code-source root (`path`), matching the | ||
| // AI Runtime's /databricks/code_source/<dir> extraction contract — the same layout | ||
| // aicode (for a directory code_source_path) and the air CLI produce. With `git` set the | ||
| // tarball snapshots that ref; otherwise it packs the working tree scoped to `include`. | ||
|
vinchenzo-db marked this conversation as resolved.
Outdated
|
||
| func buildTarballArtifact(ctx context.Context, b *bundle.Bundle, name string, a *config.Artifact) error { | ||
| if len(a.Files) != 1 { | ||
| return fmt.Errorf("artifact %q: a tgz artifact needs exactly one `files` entry naming the output path", name) | ||
| } | ||
| out := a.Files[0].Source // made absolute by artifacts.Prepare | ||
|
Contributor
There was a problem hiding this comment. Why do we need the output path to be specified? We can interpolate a temporary path, under Artifacts are uploaded separately from regular files. The places that refer to artifacts do path rewriting to make sure the final path we use in the workspace is the right one.
Contributor
Author
There was a problem hiding this comment. For now I think this is still necessary. We can mark it as a follow up if you feel it shouldn't be, but right now the BYOT API specifies I asked claude what it would take to bridge this gap and we would need to let
Contributor
There was a problem hiding this comment. Thanks, yeah I agree with the opaque bit, and that referencing via interpolation is the right solution. You'd get something like this: code_source_path: ${artifacts.air_code_source.output} |
||
| if err := os.MkdirAll(filepath.Dir(out), 0o755); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Write to a temp file and rename on success, so a failed build never leaves a | ||
| // partial tarball at `out`. | ||
| tmp, err := os.CreateTemp(filepath.Dir(out), filepath.Base(out)+".*.tmp") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| tmpName := tmp.Name() | ||
| renamed := false | ||
| defer func() { | ||
| tmp.Close() // harmless double-close after the success path; closes fd on error paths | ||
| if !renamed { | ||
| os.Remove(tmpName) | ||
|
vinchenzo-db marked this conversation as resolved.
Outdated
|
||
| } | ||
| }() | ||
|
|
||
| if a.Git != nil { | ||
| err = tarballFromGit(ctx, b, a, tmp) | ||
| } else { | ||
| err = tarballFromInclude(ctx, b, a, tmp) | ||
| } | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err := tmp.Close(); err != nil { | ||
| return err | ||
| } | ||
| if err := os.Rename(tmpName, out); err != nil { | ||
| return err | ||
| } | ||
| renamed = true | ||
| return nil | ||
| } | ||
|
|
||
| // codeRoot returns the artifact's code-source root as a sync-root-relative path plus | ||
| // its directory name. dirName is the load-bearing top-level entry the runtime extracts | ||
|
|
||
| // to /databricks/code_source/<dir>. | ||
| func codeRoot(b *bundle.Bundle, a *config.Artifact) (relBase, dirName string, err error) { | ||
| rel, err := filepath.Rel(b.SyncRootPath, a.Path) | ||
| if err != nil { | ||
| return "", "", fmt.Errorf("artifact path %q: %w", a.Path, err) | ||
| } | ||
| return filepath.ToSlash(rel), filepath.Base(a.Path), nil | ||
| } | ||
|
|
||
| // tarballFromInclude packs the working tree under the artifact's code-source root, | ||
| // optionally narrowed to `include` subpaths, using the bundle sync walker (filtering | ||
| // matches bundle file sync: .gitignore + sync.include/exclude). Entries are re-based | ||
| // under the code-source dir name via the shared aicode.BuildCodeSnapshot packer. | ||
| func tarballFromInclude(ctx context.Context, b *bundle.Bundle, a *config.Artifact, w io.Writer) error { | ||
| relBase, dirName, err := codeRoot(b, a) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| list, err := includeFiles(ctx, b, relBase, a.Include) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if len(list) == 0 { | ||
| return fmt.Errorf("artifact tgz: no files to pack under %q (all excluded by .gitignore/sync.exclude, or empty)", a.Path) | ||
| } | ||
| _, err = aicode.BuildCodeSnapshot(b.SyncRoot, relBase, list, dirName, w) | ||
|
vinchenzo-db marked this conversation as resolved.
Outdated
|
||
| return err | ||
| } | ||
|
|
||
| // includeFiles lists the files to pack: the whole code-source root (relBase) when | ||
| // include is empty, else only the given subpaths (relative to relBase). The result is | ||
| // scoped to relBase so a force-added sync.include stray outside it is dropped. | ||
| func includeFiles(ctx context.Context, b *bundle.Bundle, relBase string, include []string) ([]fileset.File, error) { | ||
| opts, err := files.GetSyncOptions(ctx, b) | ||
|
vinchenzo-db marked this conversation as resolved.
|
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| var paths []string | ||
| if len(include) == 0 { | ||
| paths = []string{relBase} | ||
| } else { | ||
| for _, p := range include { | ||
| paths = append(paths, path.Join(relBase, p)) | ||
| } | ||
| } | ||
| fl, err := libsync.NewFileList(ctx, opts.WorktreeRoot, opts.LocalRoot, paths, opts.Include, opts.Exclude) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| all, err := fl.Files(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if relBase == "." { | ||
| return all, nil | ||
| } | ||
| prefix := relBase + "/" | ||
| return slices.DeleteFunc(all, func(f fileset.File) bool { | ||
| return !strings.HasPrefix(f.Relative, prefix) | ||
| }), nil | ||
| } | ||
|
|
||
| // tarballFromGit snapshots a git ref via `git archive`, nesting every entry under the | ||
| // code-source dir name (--prefix) so the archive matches the include/aicode layout. | ||
| // Commit wins over Branch. `include`, when set, scopes the archived pathspecs. | ||
| func tarballFromGit(ctx context.Context, b *bundle.Bundle, a *config.Artifact, w io.Writer) error { | ||
|
Contributor
There was a problem hiding this comment. have we tested equivalence of tarballs from here vs CLI? Note also the tarball changes that landed in CLI recently to respect gitignore
Contributor
Author
There was a problem hiding this comment. Made them share code instead of testing equivalence |
||
| ref := a.Git.Commit | ||
| if ref == "" { | ||
| ref = a.Git.Branch | ||
| } | ||
| if ref == "" { | ||
| return errors.New("git artifact: specify git.commit or git.branch") | ||
| } | ||
| dirName := filepath.Base(a.Path) | ||
| args := []string{"-C", b.SyncRootPath, "archive", "--format=tar.gz", "--prefix=" + dirName + "/", ref} | ||
| if len(a.Include) > 0 { | ||
| args = append(args, "--") | ||
| args = append(args, a.Include...) | ||
| } | ||
| cmd := exec.CommandContext(ctx, "git", args...) | ||
| cmd.Stdout = w | ||
| var stderr strings.Builder | ||
| cmd.Stderr = &stderr | ||
| if err := cmd.Run(); err != nil { | ||
| return fmt.Errorf("git archive %s: %w: %s", ref, err, stderr.String()) | ||
| } | ||
| return nil | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| package artifacts | ||
|
|
||
| import ( | ||
| "archive/tar" | ||
| "bytes" | ||
| "compress/gzip" | ||
| "io" | ||
| "os" | ||
| "os/exec" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/databricks/cli/bundle" | ||
| "github.com/databricks/cli/bundle/config" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // tarEntries reads a gzipped tarball and returns entry name -> content. | ||
| func tarEntries(t *testing.T, b []byte) map[string]string { | ||
| t.Helper() | ||
| gzr, err := gzip.NewReader(bytes.NewReader(b)) | ||
| require.NoError(t, err) | ||
| tr := tar.NewReader(gzr) | ||
| out := map[string]string{} | ||
| for { | ||
| hdr, err := tr.Next() | ||
| if err == io.EOF { | ||
| break | ||
| } | ||
| require.NoError(t, err) | ||
| body, err := io.ReadAll(tr) | ||
| require.NoError(t, err) | ||
| out[hdr.Name] = string(body) | ||
| } | ||
| return out | ||
| } | ||
|
|
||
| func runGit(t *testing.T, dir string, args ...string) { | ||
| t.Helper() | ||
| cmd := exec.Command("git", args...) | ||
| cmd.Dir = dir | ||
| out, err := cmd.CombinedOutput() | ||
| require.NoError(t, err, "git %v: %s", args, out) | ||
| } | ||
|
|
||
| // TestTarballFromGitPrefixesEntries verifies git mode nests every entry under the | ||
| // code-source dir name (the load-bearing top-level the runtime extracts to | ||
| // /databricks/code_source/<dir>), matching aicode and the air CLI. | ||
| func TestTarballFromGitPrefixesEntries(t *testing.T) { | ||
| repo := t.TempDir() | ||
| runGit(t, repo, "init", "-q") | ||
| runGit(t, repo, "config", "user.email", "t@example.com") | ||
| runGit(t, repo, "config", "user.name", "t") | ||
| // Keep line endings verbatim so the content check doesn't depend on the runner's | ||
| // core.autocrlf (true by default on the Windows CI image). | ||
| runGit(t, repo, "config", "core.autocrlf", "false") | ||
| require.NoError(t, os.WriteFile(filepath.Join(repo, "train.py"), []byte("print('x')\n"), 0o644)) | ||
| runGit(t, repo, "add", "-A") | ||
| runGit(t, repo, "commit", "-qm", "init") | ||
|
|
||
| b := &bundle.Bundle{SyncRootPath: repo} | ||
| a := &config.Artifact{Path: repo, Git: &config.ArtifactGit{Commit: "HEAD"}} | ||
|
|
||
| var buf bytes.Buffer | ||
| require.NoError(t, tarballFromGit(t.Context(), b, a, &buf)) | ||
|
|
||
| // The entry nests under the code-source dir name (the load-bearing top-level). | ||
| dir := filepath.Base(repo) | ||
| entries := tarEntries(t, buf.Bytes()) | ||
| require.Contains(t, entries, dir+"/train.py") | ||
| assert.Equal(t, "print('x')", strings.TrimSpace(entries[dir+"/train.py"])) | ||
| } | ||
|
|
||
| func TestTarballFromGitRequiresRef(t *testing.T) { | ||
| b := &bundle.Bundle{SyncRootPath: t.TempDir()} | ||
| a := &config.Artifact{Path: b.SyncRootPath, Git: &config.ArtifactGit{}} | ||
| err := tarballFromGit(t.Context(), b, a, io.Discard) | ||
| require.ErrorContains(t, err, "git.commit or git.branch") | ||
| } |
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is semantics for
includethe same as in air cli? There it is relative to code source root, is include relative to bundle root in DABs? Can we make sure that is documented well somewhere?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include is now relative to the code-source root, same as the air CLI. The reason I decided this is so that
convert-to-dabscan map air CLI'sinclude_pathsstraight across later.