Skip to content

Commit b0b9394

Browse files
authored
Fix trampoline notebook filename collision (#5508)
## Why Found during a full-repo review of the CLI. The python wheel wrapper (trampoline) names its generated notebook `notebook_<job_key>_<task_key>`. Both keys can contain underscores, so distinct pairs can produce the same name: job `a_b` with task `c` and job `a` with task `b_c` both map to `notebook_a_b_c`. The second wrapper overwrites the first, and both tasks point at the same notebook, so one task silently runs the other task's wrapper. ## Changes Before, two different (job key, task key) pairs could collide on one wrapper filename; now every pair gets a unique name. The name is now `notebook_<job_key_length>_<job_key>_<task_key>` (in `bundle/trampoline/trampoline.go`). The length prefix makes the encoding unambiguous for any keys, with no new dependencies. The example above becomes `notebook_3_a_b_c` vs `notebook_1_a_b_c`. Existing wrapper notebooks get a new path on the next deploy. This only affects bundles with `experimental.python_wheel_wrapper: true`, and the wrapper is regenerated and the job spec updated on every deploy, so no action is needed. ## Test plan - [x] New unit test `TestGenerateTrampolineWithCollidingKeys` covering the colliding pair, asserting two distinct wrapper files and notebook paths (fails without the fix) - [x] Updated existing expectations in `bundle/trampoline` tests for the new naming - [x] `go test ./bundle/trampoline/ -count=1` passes - [x] `./task fmt-q`, `./task lint-q`, and `./task checks` pass This pull request and its description were written by Isaac.
1 parent bf1d217 commit b0b9394

3 files changed

Lines changed: 73 additions & 9 deletions

File tree

bundle/trampoline/conditional_transform_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ func TestTransformWithExperimentalSettingSetToTrue(t *testing.T) {
108108
task := b.Config.Resources.Jobs["job1"].Tasks[0]
109109
require.Nil(t, task.PythonWheelTask)
110110
require.NotNil(t, task.NotebookTask)
111-
require.Equal(t, "/Workspace/files/my_bundle/.databricks/bundle/development/.internal/notebook_job1_key1", task.NotebookTask.NotebookPath)
111+
require.Equal(t, "/Workspace/files/my_bundle/.databricks/bundle/development/.internal/notebook_4_job1_key1", task.NotebookTask.NotebookPath)
112112

113113
require.Len(t, task.Libraries, 1)
114114
require.Equal(t, "/Workspace/Users/test@test.com/bundle/dist/test.jar", task.Libraries[0].Jar)

bundle/trampoline/trampoline.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,9 @@ func (m *trampoline) generateNotebookWrapper(ctx context.Context, b *bundle.Bund
5959
return err
6060
}
6161

62-
notebookName := fmt.Sprintf("notebook_%s_%s", task.JobKey, task.Task.TaskKey)
62+
// Keys may contain underscores, so joining with "_" alone is ambiguous ("a_b"+"c"
63+
// vs "a"+"b_c"); the job key length prefix keeps the filename unique per task.
64+
notebookName := fmt.Sprintf("notebook_%d_%s_%s", len(task.JobKey), task.JobKey, task.Task.TaskKey)
6365
localNotebookPath := filepath.Join(internalDir, notebookName+".py")
6466

6567
err = os.MkdirAll(filepath.Dir(localNotebookPath), 0o755)

bundle/trampoline/trampoline_test.go

Lines changed: 69 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,13 @@ type functions struct{}
1717

1818
func (f *functions) GetTasks(b *bundle.Bundle) []TaskWithJobKey {
1919
var tasks []TaskWithJobKey
20-
for k := range b.Config.Resources.Jobs["test"].Tasks {
21-
tasks = append(tasks, TaskWithJobKey{
22-
JobKey: "test",
23-
Task: &b.Config.Resources.Jobs["test"].Tasks[k],
24-
})
20+
for k, job := range b.Config.Resources.Jobs {
21+
for i := range job.Tasks {
22+
tasks = append(tasks, TaskWithJobKey{
23+
JobKey: k,
24+
Task: &job.Tasks[i],
25+
})
26+
}
2527
}
2628

2729
return tasks
@@ -85,14 +87,74 @@ func TestGenerateTrampoline(t *testing.T) {
8587

8688
dir, err := b.InternalDir(ctx)
8789
require.NoError(t, err)
88-
filename := filepath.Join(dir, "notebook_test_to_trampoline.py")
90+
filename := filepath.Join(dir, "notebook_4_test_to_trampoline.py")
8991

9092
bytes, err := os.ReadFile(filename)
9193
require.NoError(t, err)
9294

9395
require.Equal(t, "Hello from Trampoline", string(bytes))
9496

9597
task := b.Config.Resources.Jobs["test"].Tasks[0]
96-
require.Equal(t, "/Workspace/files/my_bundle/.databricks/bundle/development/.internal/notebook_test_to_trampoline", task.NotebookTask.NotebookPath)
98+
require.Equal(t, "/Workspace/files/my_bundle/.databricks/bundle/development/.internal/notebook_4_test_to_trampoline", task.NotebookTask.NotebookPath)
9799
require.Nil(t, task.PythonWheelTask)
98100
}
101+
102+
func TestGenerateTrampolineWithCollidingKeys(t *testing.T) {
103+
tmpDir := t.TempDir()
104+
105+
newJob := func(taskKey string) *resources.Job {
106+
return &resources.Job{
107+
JobSettings: jobs.JobSettings{
108+
Tasks: []jobs.Task{
109+
{
110+
TaskKey: taskKey,
111+
PythonWheelTask: &jobs.PythonWheelTask{
112+
PackageName: "test",
113+
EntryPoint: "run",
114+
},
115+
},
116+
},
117+
},
118+
}
119+
}
120+
121+
b := &bundle.Bundle{
122+
BundleRootPath: filepath.Join(tmpDir, "parent", "my_bundle"),
123+
SyncRootPath: filepath.Join(tmpDir, "parent"),
124+
Config: config.Root{
125+
Workspace: config.Workspace{
126+
FilePath: "/Workspace/files",
127+
},
128+
Bundle: config.Bundle{
129+
Target: "development",
130+
},
131+
Resources: config.Resources{
132+
Jobs: map[string]*resources.Job{
133+
"a_b": newJob("c"),
134+
"a": newJob("b_c"),
135+
},
136+
},
137+
},
138+
}
139+
ctx := t.Context()
140+
141+
funcs := functions{}
142+
trampoline := NewTrampoline("test_trampoline", &funcs, "Hello from {{.MyName}}")
143+
diags := bundle.Apply(ctx, b, trampoline)
144+
require.NoError(t, diags.Error())
145+
146+
dir, err := b.InternalDir(ctx)
147+
require.NoError(t, err)
148+
149+
for _, name := range []string{"notebook_3_a_b_c", "notebook_1_a_b_c"} {
150+
_, err := os.Stat(filepath.Join(dir, name+".py"))
151+
require.NoError(t, err)
152+
}
153+
154+
require.Equal(t,
155+
"/Workspace/files/my_bundle/.databricks/bundle/development/.internal/notebook_3_a_b_c",
156+
b.Config.Resources.Jobs["a_b"].Tasks[0].NotebookTask.NotebookPath)
157+
require.Equal(t,
158+
"/Workspace/files/my_bundle/.databricks/bundle/development/.internal/notebook_1_a_b_c",
159+
b.Config.Resources.Jobs["a"].Tasks[0].NotebookTask.NotebookPath)
160+
}

0 commit comments

Comments
 (0)