From c6b03ba6d53b23b563fec6804ac97cf9da9d10d8 Mon Sep 17 00:00:00 2001 From: Witold Czaplewski Date: Mon, 1 Jul 2024 09:02:53 +0200 Subject: [PATCH 1/5] Add support for requirement libraries --- bundle/libraries/helpers.go | 3 +++ bundle/libraries/helpers_test.go | 1 + 2 files changed, 4 insertions(+) diff --git a/bundle/libraries/helpers.go b/bundle/libraries/helpers.go index 89679c91a23..b7e707ccf8d 100644 --- a/bundle/libraries/helpers.go +++ b/bundle/libraries/helpers.go @@ -12,5 +12,8 @@ func libraryPath(library *compute.Library) string { if library.Egg != "" { return library.Egg } + if library.Requirements != "" { + return library.Requirements + } return "" } diff --git a/bundle/libraries/helpers_test.go b/bundle/libraries/helpers_test.go index adc20a246bf..57300561a14 100644 --- a/bundle/libraries/helpers_test.go +++ b/bundle/libraries/helpers_test.go @@ -13,5 +13,6 @@ func TestLibraryPath(t *testing.T) { assert.Equal(t, path, libraryPath(&compute.Library{Whl: path})) assert.Equal(t, path, libraryPath(&compute.Library{Jar: path})) assert.Equal(t, path, libraryPath(&compute.Library{Egg: path})) + assert.Equal(t, path, libraryPath(&compute.Library{Requirements: path})) assert.Equal(t, "", libraryPath(&compute.Library{})) } From 4e657fce70d6e7fb6dffa42d250250b297615390 Mon Sep 17 00:00:00 2001 From: Witold Czaplewski Date: Mon, 8 Jul 2024 08:55:58 +0200 Subject: [PATCH 2/5] Fix format --- bundle/libraries/helpers_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundle/libraries/helpers_test.go b/bundle/libraries/helpers_test.go index 57300561a14..e4bd32770b0 100644 --- a/bundle/libraries/helpers_test.go +++ b/bundle/libraries/helpers_test.go @@ -13,6 +13,6 @@ func TestLibraryPath(t *testing.T) { assert.Equal(t, path, libraryPath(&compute.Library{Whl: path})) assert.Equal(t, path, libraryPath(&compute.Library{Jar: path})) assert.Equal(t, path, libraryPath(&compute.Library{Egg: path})) - assert.Equal(t, path, libraryPath(&compute.Library{Requirements: path})) + assert.Equal(t, path, libraryPath(&compute.Library{Requirements: path})) assert.Equal(t, "", libraryPath(&compute.Library{})) } From cce6c40bb09f2089978c4d370dae654442045923 Mon Sep 17 00:00:00 2001 From: Witold Czaplewski Date: Mon, 29 Jul 2024 11:46:06 +0200 Subject: [PATCH 3/5] Add support for local requirement files --- bundle/artifacts/artifacts.go | 6 ++++++ bundle/artifacts/artifacts_test.go | 8 ++++++++ bundle/config/mutator/translate_paths_jobs.go | 5 +++++ 3 files changed, 19 insertions(+) diff --git a/bundle/artifacts/artifacts.go b/bundle/artifacts/artifacts.go index 3060d08d9e7..56cba3aa019 100644 --- a/bundle/artifacts/artifacts.go +++ b/bundle/artifacts/artifacts.go @@ -186,6 +186,9 @@ func rewriteArtifactPath(b *bundle.Bundle, f *config.ArtifactFile, job *resource if lib.Jar != "" && isArtifactMatchLibrary(f, lib.Jar, b) { lib.Jar = remotePath } + if lib.Requirements != "" && isArtifactMatchLibrary(f, lib.Requirements, b) { + lib.Requirements = remotePath + } } // Rewrite artifact path in job task libraries for ForEachTask @@ -199,6 +202,9 @@ func rewriteArtifactPath(b *bundle.Bundle, f *config.ArtifactFile, job *resource if lib.Jar != "" && isArtifactMatchLibrary(f, lib.Jar, b) { lib.Jar = remotePath } + if lib.Requirements != "" && isArtifactMatchLibrary(f, lib.Requirements, b) { + lib.Requirements = remotePath + } } } } diff --git a/bundle/artifacts/artifacts_test.go b/bundle/artifacts/artifacts_test.go index 6d85f3af908..19724d2baab 100644 --- a/bundle/artifacts/artifacts_test.go +++ b/bundle/artifacts/artifacts_test.go @@ -50,6 +50,9 @@ func TestArtifactUploadForWorkspace(t *testing.T) { { Whl: "/Workspace/Users/foo@bar.com/mywheel.whl", }, + { + Requirements: "/Workspace/Users/foo@bar.com/requirements.txt", + }, }, }, { @@ -62,6 +65,9 @@ func TestArtifactUploadForWorkspace(t *testing.T) { { Whl: "/Workspace/Users/foo@bar.com/mywheel.whl", }, + { + Requirements: "/Workspace/Users/foo@bar.com/requirements.txt", + }, }, }, }, @@ -100,10 +106,12 @@ func TestArtifactUploadForWorkspace(t *testing.T) { // Test that libraries path is updated require.Equal(t, "/Workspace/foo/bar/artifacts/source.whl", b.Config.Resources.Jobs["job"].JobSettings.Tasks[0].Libraries[0].Whl) require.Equal(t, "/Workspace/Users/foo@bar.com/mywheel.whl", b.Config.Resources.Jobs["job"].JobSettings.Tasks[0].Libraries[1].Whl) + require.Equal(t, "/Workspace/Users/foo@bar.com/requirements.txt", b.Config.Resources.Jobs["job"].JobSettings.Tasks[0].Libraries[2].Requirements) require.Equal(t, "/Workspace/foo/bar/artifacts/source.whl", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[0]) require.Equal(t, "/Workspace/Users/foo@bar.com/mywheel.whl", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[1]) require.Equal(t, "/Workspace/foo/bar/artifacts/source.whl", b.Config.Resources.Jobs["job"].JobSettings.Tasks[1].ForEachTask.Task.Libraries[0].Whl) require.Equal(t, "/Workspace/Users/foo@bar.com/mywheel.whl", b.Config.Resources.Jobs["job"].JobSettings.Tasks[1].ForEachTask.Task.Libraries[1].Whl) + require.Equal(t, "/Workspace/Users/foo@bar.com/requirements.txt", b.Config.Resources.Jobs["job"].JobSettings.Tasks[1].ForEachTask.Task.Libraries[2].Requirements) } func TestArtifactUploadForVolumes(t *testing.T) { diff --git a/bundle/config/mutator/translate_paths_jobs.go b/bundle/config/mutator/translate_paths_jobs.go index 60cc8bb9aa4..b1d890b098b 100644 --- a/bundle/config/mutator/translate_paths_jobs.go +++ b/bundle/config/mutator/translate_paths_jobs.go @@ -50,6 +50,11 @@ func rewritePatterns(t *translateContext, base dyn.Pattern) []jobRewritePattern t.translateNoOp, noSkipRewrite, }, + { + base.Append(dyn.Key("libraries"), dyn.AnyIndex(), dyn.Key("requirements")), + t.translateNoOp, + noSkipRewrite, + }, } } From 8337b265976a9c9700576e7279f7d65ca368c9fb Mon Sep 17 00:00:00 2001 From: Witold Czaplewski Date: Tue, 30 Jul 2024 11:20:46 +0200 Subject: [PATCH 4/5] Adapt artifacts and translation according to review --- bundle/artifacts/artifacts.go | 4 +- bundle/artifacts/artifacts_test.go | 39 ++++++++++++++----- bundle/config/artifact.go | 1 + bundle/config/mutator/translate_paths_jobs.go | 2 +- bundle/config/mutator/translate_paths_test.go | 9 +++++ 5 files changed, 43 insertions(+), 12 deletions(-) diff --git a/bundle/artifacts/artifacts.go b/bundle/artifacts/artifacts.go index 56cba3aa019..c709312d268 100644 --- a/bundle/artifacts/artifacts.go +++ b/bundle/artifacts/artifacts.go @@ -186,7 +186,7 @@ func rewriteArtifactPath(b *bundle.Bundle, f *config.ArtifactFile, job *resource if lib.Jar != "" && isArtifactMatchLibrary(f, lib.Jar, b) { lib.Jar = remotePath } - if lib.Requirements != "" && isArtifactMatchLibrary(f, lib.Requirements, b) { + if lib.Requirements != "" { lib.Requirements = remotePath } } @@ -202,7 +202,7 @@ func rewriteArtifactPath(b *bundle.Bundle, f *config.ArtifactFile, job *resource if lib.Jar != "" && isArtifactMatchLibrary(f, lib.Jar, b) { lib.Jar = remotePath } - if lib.Requirements != "" && isArtifactMatchLibrary(f, lib.Requirements, b) { + if lib.Requirements != "" { lib.Requirements = remotePath } } diff --git a/bundle/artifacts/artifacts_test.go b/bundle/artifacts/artifacts_test.go index 19724d2baab..370900d52ba 100644 --- a/bundle/artifacts/artifacts_test.go +++ b/bundle/artifacts/artifacts_test.go @@ -23,6 +23,10 @@ func TestArtifactUploadForWorkspace(t *testing.T) { testutil.Touch(t, whlFolder, "source.whl") whlLocalPath := filepath.Join(whlFolder, "source.whl") + requirementsFolder := filepath.Join(tmpDir, "requirements") + testutil.Touch(t, requirementsFolder, "requirements.txt") + requirementsLocalPath := filepath.Join(requirementsFolder, "requirements.txt") + b := &bundle.Bundle{ RootPath: tmpDir, Config: config.Root{ @@ -36,6 +40,12 @@ func TestArtifactUploadForWorkspace(t *testing.T) { {Source: whlLocalPath}, }, }, + "requirements": { + Type: config.ArtifactPythonRequirementsFile, + Files: []config.ArtifactFile{ + {Source: requirementsLocalPath}, + }, + }, }, Resources: config.Resources{ Jobs: map[string]*resources.Job{ @@ -51,7 +61,7 @@ func TestArtifactUploadForWorkspace(t *testing.T) { Whl: "/Workspace/Users/foo@bar.com/mywheel.whl", }, { - Requirements: "/Workspace/Users/foo@bar.com/requirements.txt", + Requirements: filepath.Join("requirements", "requirements.txt"), }, }, }, @@ -66,7 +76,7 @@ func TestArtifactUploadForWorkspace(t *testing.T) { Whl: "/Workspace/Users/foo@bar.com/mywheel.whl", }, { - Requirements: "/Workspace/Users/foo@bar.com/requirements.txt", + Requirements: filepath.Join("requirements", "requirements.txt"), }, }, }, @@ -90,9 +100,8 @@ func TestArtifactUploadForWorkspace(t *testing.T) { }, } - artifact := b.Config.Artifacts["whl"] - mockFiler := mockfiler.NewMockFiler(t) - mockFiler.EXPECT().Write( + whlMockFiler := mockfiler.NewMockFiler(t) + whlMockFiler.EXPECT().Write( mock.Anything, filepath.Join("source.whl"), mock.AnythingOfType("*bytes.Reader"), @@ -100,18 +109,30 @@ func TestArtifactUploadForWorkspace(t *testing.T) { filer.CreateParentDirectories, ).Return(nil) - err := uploadArtifact(context.Background(), b, artifact, "/foo/bar/artifacts", mockFiler) - require.NoError(t, err) + whlError := uploadArtifact(context.Background(), b, b.Config.Artifacts["whl"], "/foo/bar/artifacts", whlMockFiler) + require.NoError(t, whlError) + + requirementsMockFiler := mockfiler.NewMockFiler(t) + requirementsMockFiler.EXPECT().Write( + mock.Anything, + filepath.Join("requirements.txt"), + mock.AnythingOfType("*bytes.Reader"), + filer.OverwriteIfExists, + filer.CreateParentDirectories, + ).Return(nil) + + requirementsError := uploadArtifact(context.Background(), b, b.Config.Artifacts["requirements"], "/foo/bar/artifacts", requirementsMockFiler) + require.NoError(t, requirementsError) // Test that libraries path is updated require.Equal(t, "/Workspace/foo/bar/artifacts/source.whl", b.Config.Resources.Jobs["job"].JobSettings.Tasks[0].Libraries[0].Whl) require.Equal(t, "/Workspace/Users/foo@bar.com/mywheel.whl", b.Config.Resources.Jobs["job"].JobSettings.Tasks[0].Libraries[1].Whl) - require.Equal(t, "/Workspace/Users/foo@bar.com/requirements.txt", b.Config.Resources.Jobs["job"].JobSettings.Tasks[0].Libraries[2].Requirements) + require.Equal(t, "/Workspace/foo/bar/artifacts/requirements.txt", b.Config.Resources.Jobs["job"].JobSettings.Tasks[0].Libraries[2].Requirements) require.Equal(t, "/Workspace/foo/bar/artifacts/source.whl", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[0]) require.Equal(t, "/Workspace/Users/foo@bar.com/mywheel.whl", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[1]) require.Equal(t, "/Workspace/foo/bar/artifacts/source.whl", b.Config.Resources.Jobs["job"].JobSettings.Tasks[1].ForEachTask.Task.Libraries[0].Whl) require.Equal(t, "/Workspace/Users/foo@bar.com/mywheel.whl", b.Config.Resources.Jobs["job"].JobSettings.Tasks[1].ForEachTask.Task.Libraries[1].Whl) - require.Equal(t, "/Workspace/Users/foo@bar.com/requirements.txt", b.Config.Resources.Jobs["job"].JobSettings.Tasks[1].ForEachTask.Task.Libraries[2].Requirements) + require.Equal(t, "/Workspace/foo/bar/artifacts/requirements.txt", b.Config.Resources.Jobs["job"].JobSettings.Tasks[1].ForEachTask.Task.Libraries[2].Requirements) } func TestArtifactUploadForVolumes(t *testing.T) { diff --git a/bundle/config/artifact.go b/bundle/config/artifact.go index 219def5714d..05d61c75842 100644 --- a/bundle/config/artifact.go +++ b/bundle/config/artifact.go @@ -19,6 +19,7 @@ func (artifacts Artifacts) ConfigureConfigFilePath() { type ArtifactType string const ArtifactPythonWheel ArtifactType = `whl` +const ArtifactPythonRequirementsFile ArtifactType = `requirements` type ArtifactFile struct { Source string `json:"source"` diff --git a/bundle/config/mutator/translate_paths_jobs.go b/bundle/config/mutator/translate_paths_jobs.go index b1d890b098b..4a38958793f 100644 --- a/bundle/config/mutator/translate_paths_jobs.go +++ b/bundle/config/mutator/translate_paths_jobs.go @@ -52,7 +52,7 @@ func rewritePatterns(t *translateContext, base dyn.Pattern) []jobRewritePattern }, { base.Append(dyn.Key("libraries"), dyn.AnyIndex(), dyn.Key("requirements")), - t.translateNoOp, + t.translateFilePath, noSkipRewrite, }, } diff --git a/bundle/config/mutator/translate_paths_test.go b/bundle/config/mutator/translate_paths_test.go index 780a540df02..fd64593be46 100644 --- a/bundle/config/mutator/translate_paths_test.go +++ b/bundle/config/mutator/translate_paths_test.go @@ -110,6 +110,7 @@ func TestTranslatePaths(t *testing.T) { touchNotebookFile(t, filepath.Join(dir, "my_pipeline_notebook.py")) touchEmptyFile(t, filepath.Join(dir, "my_python_file.py")) touchEmptyFile(t, filepath.Join(dir, "dist", "task.jar")) + touchEmptyFile(t, filepath.Join(dir, "requirements.txt")) b := &bundle.Bundle{ RootPath: dir, @@ -140,6 +141,9 @@ func TestTranslatePaths(t *testing.T) { NotebookTask: &jobs.NotebookTask{ NotebookPath: "./my_job_notebook.py", }, + Libraries: []compute.Library{ + {Requirements: "./requirements.txt"}, + }, }, { PythonWheelTask: &jobs.PythonWheelTask{ @@ -232,6 +236,11 @@ func TestTranslatePaths(t *testing.T) { "/bundle/my_job_notebook", b.Config.Resources.Jobs["job"].Tasks[2].NotebookTask.NotebookPath, ) + assert.Equal( + t, + "/bundle/requirements.txt", + b.Config.Resources.Jobs["job"].Tasks[2].Libraries[0].Requirements, + ) assert.Equal( t, "/bundle/my_python_file.py", From 088b549e64e918ee6775765aca01b308e4fd6117 Mon Sep 17 00:00:00 2001 From: Witold Czaplewski Date: Tue, 30 Jul 2024 14:40:15 +0200 Subject: [PATCH 5/5] Revert changes to artifacts --- bundle/artifacts/artifacts.go | 6 ----- bundle/artifacts/artifacts_test.go | 39 ++++-------------------------- bundle/config/artifact.go | 1 - 3 files changed, 5 insertions(+), 41 deletions(-) diff --git a/bundle/artifacts/artifacts.go b/bundle/artifacts/artifacts.go index c709312d268..3060d08d9e7 100644 --- a/bundle/artifacts/artifacts.go +++ b/bundle/artifacts/artifacts.go @@ -186,9 +186,6 @@ func rewriteArtifactPath(b *bundle.Bundle, f *config.ArtifactFile, job *resource if lib.Jar != "" && isArtifactMatchLibrary(f, lib.Jar, b) { lib.Jar = remotePath } - if lib.Requirements != "" { - lib.Requirements = remotePath - } } // Rewrite artifact path in job task libraries for ForEachTask @@ -202,9 +199,6 @@ func rewriteArtifactPath(b *bundle.Bundle, f *config.ArtifactFile, job *resource if lib.Jar != "" && isArtifactMatchLibrary(f, lib.Jar, b) { lib.Jar = remotePath } - if lib.Requirements != "" { - lib.Requirements = remotePath - } } } } diff --git a/bundle/artifacts/artifacts_test.go b/bundle/artifacts/artifacts_test.go index 370900d52ba..6d85f3af908 100644 --- a/bundle/artifacts/artifacts_test.go +++ b/bundle/artifacts/artifacts_test.go @@ -23,10 +23,6 @@ func TestArtifactUploadForWorkspace(t *testing.T) { testutil.Touch(t, whlFolder, "source.whl") whlLocalPath := filepath.Join(whlFolder, "source.whl") - requirementsFolder := filepath.Join(tmpDir, "requirements") - testutil.Touch(t, requirementsFolder, "requirements.txt") - requirementsLocalPath := filepath.Join(requirementsFolder, "requirements.txt") - b := &bundle.Bundle{ RootPath: tmpDir, Config: config.Root{ @@ -40,12 +36,6 @@ func TestArtifactUploadForWorkspace(t *testing.T) { {Source: whlLocalPath}, }, }, - "requirements": { - Type: config.ArtifactPythonRequirementsFile, - Files: []config.ArtifactFile{ - {Source: requirementsLocalPath}, - }, - }, }, Resources: config.Resources{ Jobs: map[string]*resources.Job{ @@ -60,9 +50,6 @@ func TestArtifactUploadForWorkspace(t *testing.T) { { Whl: "/Workspace/Users/foo@bar.com/mywheel.whl", }, - { - Requirements: filepath.Join("requirements", "requirements.txt"), - }, }, }, { @@ -75,9 +62,6 @@ func TestArtifactUploadForWorkspace(t *testing.T) { { Whl: "/Workspace/Users/foo@bar.com/mywheel.whl", }, - { - Requirements: filepath.Join("requirements", "requirements.txt"), - }, }, }, }, @@ -100,8 +84,9 @@ func TestArtifactUploadForWorkspace(t *testing.T) { }, } - whlMockFiler := mockfiler.NewMockFiler(t) - whlMockFiler.EXPECT().Write( + artifact := b.Config.Artifacts["whl"] + mockFiler := mockfiler.NewMockFiler(t) + mockFiler.EXPECT().Write( mock.Anything, filepath.Join("source.whl"), mock.AnythingOfType("*bytes.Reader"), @@ -109,30 +94,16 @@ func TestArtifactUploadForWorkspace(t *testing.T) { filer.CreateParentDirectories, ).Return(nil) - whlError := uploadArtifact(context.Background(), b, b.Config.Artifacts["whl"], "/foo/bar/artifacts", whlMockFiler) - require.NoError(t, whlError) - - requirementsMockFiler := mockfiler.NewMockFiler(t) - requirementsMockFiler.EXPECT().Write( - mock.Anything, - filepath.Join("requirements.txt"), - mock.AnythingOfType("*bytes.Reader"), - filer.OverwriteIfExists, - filer.CreateParentDirectories, - ).Return(nil) - - requirementsError := uploadArtifact(context.Background(), b, b.Config.Artifacts["requirements"], "/foo/bar/artifacts", requirementsMockFiler) - require.NoError(t, requirementsError) + err := uploadArtifact(context.Background(), b, artifact, "/foo/bar/artifacts", mockFiler) + require.NoError(t, err) // Test that libraries path is updated require.Equal(t, "/Workspace/foo/bar/artifacts/source.whl", b.Config.Resources.Jobs["job"].JobSettings.Tasks[0].Libraries[0].Whl) require.Equal(t, "/Workspace/Users/foo@bar.com/mywheel.whl", b.Config.Resources.Jobs["job"].JobSettings.Tasks[0].Libraries[1].Whl) - require.Equal(t, "/Workspace/foo/bar/artifacts/requirements.txt", b.Config.Resources.Jobs["job"].JobSettings.Tasks[0].Libraries[2].Requirements) require.Equal(t, "/Workspace/foo/bar/artifacts/source.whl", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[0]) require.Equal(t, "/Workspace/Users/foo@bar.com/mywheel.whl", b.Config.Resources.Jobs["job"].JobSettings.Environments[0].Spec.Dependencies[1]) require.Equal(t, "/Workspace/foo/bar/artifacts/source.whl", b.Config.Resources.Jobs["job"].JobSettings.Tasks[1].ForEachTask.Task.Libraries[0].Whl) require.Equal(t, "/Workspace/Users/foo@bar.com/mywheel.whl", b.Config.Resources.Jobs["job"].JobSettings.Tasks[1].ForEachTask.Task.Libraries[1].Whl) - require.Equal(t, "/Workspace/foo/bar/artifacts/requirements.txt", b.Config.Resources.Jobs["job"].JobSettings.Tasks[1].ForEachTask.Task.Libraries[2].Requirements) } func TestArtifactUploadForVolumes(t *testing.T) { diff --git a/bundle/config/artifact.go b/bundle/config/artifact.go index 05d61c75842..219def5714d 100644 --- a/bundle/config/artifact.go +++ b/bundle/config/artifact.go @@ -19,7 +19,6 @@ func (artifacts Artifacts) ConfigureConfigFilePath() { type ArtifactType string const ArtifactPythonWheel ArtifactType = `whl` -const ArtifactPythonRequirementsFile ArtifactType = `requirements` type ArtifactFile struct { Source string `json:"source"`