Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 214
Correctly translate paths to local requirements.txt file in environment dependencies#2736
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
9c790960f29b6c8a5babbee64e50b3be69432fcdf6269d171File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| bundle: | ||
| name: "dependencies" | ||
| resources: | ||
| jobs: | ||
| test_job: | ||
| name: "Test Job" | ||
| tasks: | ||
| - task_key: "main" | ||
| python_wheel_task: | ||
| package_name: "test_package" | ||
| entry_point: "main" | ||
| environment_key: "test_env" | ||
| environments: | ||
| - environment_key: "test_env" | ||
| spec: | ||
| client: "1" | ||
| dependencies: | ||
| - "-r ./requirements.txt" | ||
| - "test_package" | ||
| - "test_package==2.0.1" | ||
| - "test_package>=2.0.1" | ||
| - "dist/*.whl" | ||
| - "/Workspace/Users/test@databricks.com/test-package.whl" | ||
| - "beautifulsoup4>=1.0.0,~=1.2.0,<2.0.0" | ||
| - "beautifulsoup4[security, tests] ~= 4.12.3" | ||
| - "requests[security] @ https://github.com/psf/requests/archive/refs/heads/main.zip" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| >>> [CLI] bundle validate | ||
| Name: dependencies | ||
| Target: default | ||
| Workspace: | ||
| User: [USERNAME] | ||
| Path: /Workspace/Users/[USERNAME]/.bundle/dependencies/default | ||
| Validation OK! | ||
| >>> [CLI] bundle deploy | ||
| Uploading dist/test.whl... | ||
| Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/dependencies/default/files... | ||
| Deploying resources... | ||
| Updating deployment state... | ||
| Deployment complete! | ||
| >>> jq -s .[] | select(.path=="/api/2.2/jobs/create") | .body.environments out.requests.txt | ||
| [ | ||
| { | ||
| "environment_key": "test_env", | ||
| "spec": { | ||
| "client": "1", | ||
| "dependencies": [ | ||
| "-r /Workspace/Users/[USERNAME]/.bundle/dependencies/default/files/requirements.txt", | ||
| "test_package", | ||
| "test_package==2.0.1", | ||
| "test_package>=2.0.1", | ||
| "/Workspace/Users/[USERNAME]/.bundle/dependencies/default/artifacts/.internal/test.whl", | ||
| "/Workspace/Users/test@databricks.com/test-package.whl", | ||
| "beautifulsoup4>=1.0.0,~=1.2.0,<2.0.0", | ||
| "beautifulsoup4[security, tests] ~= 4.12.3", | ||
| "requests[security] @ https://github.com/psf/requests/archive/refs/heads/main.zip" | ||
| ] | ||
| } | ||
| } | ||
| ] | ||
| >>> [CLI] bundle validate -o json | ||
| [ | ||
| "-r /Workspace/Users/[USERNAME]/.bundle/dependencies/default/files/requirements.txt", | ||
| "test_package", | ||
| "test_package==2.0.1", | ||
| "test_package>=2.0.1", | ||
| "./dist/*.whl", | ||
| "/Workspace/Users/test@databricks.com/test-package.whl", | ||
| "beautifulsoup4>=1.0.0,~=1.2.0,<2.0.0", | ||
| "beautifulsoup4[security, tests] ~= 4.12.3", | ||
| "requests[security] @ https://github.com/psf/requests/archive/refs/heads/main.zip" | ||
| ] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Flask==2.0.1 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| trace $CLI bundle validate | ||
| trace $CLI bundle deploy | ||
| trace jq -s '.[] | select(.path=="/api/2.2/jobs/create") | .body.environments' out.requests.txt | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: do you assert on the request payload because it includes the expanded wheel path? ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, exactly | ||
| trace $CLI bundle validate -o json | jq '.resources.jobs.test_job.environments[0].spec.dependencies' | ||
| rm out.requests.txt | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Cloud = false | ||
| RecordRequests = true |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -85,9 +85,30 @@ func jobRewritePatterns() []jobRewritePattern { | ||
| }, | ||
| } | ||
| jobEnvironmentsWithRequirementsPatterns := []jobRewritePattern{ | ||
| { | ||
| dyn.NewPattern( | ||
| dyn.Key("resources"), | ||
| dyn.Key("jobs"), | ||
| dyn.AnyKey(), | ||
| dyn.Key("environments"), | ||
| dyn.AnyIndex(), | ||
| dyn.Key("spec"), | ||
| dyn.Key("dependencies"), | ||
| dyn.AnyIndex(), | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Btw, all patterns in this path visitor package can be rewritten. | ||
| ), | ||
| TranslateModeEnvironmentRequirements, | ||
| func(s string) bool { | ||
| _, ok := libraries.IsLocalRequirementsFile(s) | ||
| return !ok | ||
| }, | ||
| }, | ||
| } | ||
| taskPatterns := jobTaskRewritePatterns(base) | ||
| forEachPatterns := jobTaskRewritePatterns(base.Append(dyn.Key("for_each_task"), dyn.Key("task"))) | ||
| allPatterns := append(taskPatterns, jobEnvironmentsPatterns...) | ||
| allPatterns = append(allPatterns, jobEnvironmentsWithRequirementsPatterns...) | ||
| allPatterns = append(allPatterns, forEachPatterns...) | ||
| return allPatterns | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -64,7 +64,10 @@ func IsLibraryLocal(dep string) bool { | ||
| return false | ||
| } | ||
| // If the dependency is a requirements file, it's not a valid local path | ||
| // If the dependency is a requirements file, it can either be a local path or a remote path. | ||
| // Even though the path to requirements.txt can be local we don't return true in this function anyway | ||
| // and don't treat such path as a local library path. | ||
| // Instead we handle translation of these paths in a separate code path in TranslatePath mutator. | ||
| if strings.HasPrefix(dep, "-r") { | ||
| return false | ||
| } | ||
| @@ -77,6 +80,16 @@ func IsLibraryLocal(dep string) bool { | ||
| return IsLocalPath(dep) | ||
| } | ||
| func IsLocalRequirementsFile(dep string) (string, bool) { | ||
| dep, ok := strings.CutPrefix(dep, "-r ") | ||
| if !ok { | ||
| return "", false | ||
| } | ||
| dep = strings.TrimSpace(dep) | ||
| return dep, IsLocalPath(dep) | ||
| } | ||
andrewnester marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| func containsPipFlag(input string) bool { | ||
| re := regexp.MustCompile(`--[a-zA-Z0-9-]+`) | ||
| return re.MatchString(input) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -52,6 +52,7 @@ func TestIsLibraryLocal(t *testing.T) { | ||
| {path: "pypipackage", expected: false}, | ||
| {path: "/Volumes/catalog/schema/volume/path.whl", expected: false}, | ||
| {path: "/Workspace/my_project/dist.whl", expected: false}, | ||
| {path: "-r ../requirements.txt", expected: false}, | ||
| {path: "-r /Workspace/my_project/requirements.txt", expected: false}, | ||
| {path: "s3://mybucket/path/to/package", expected: false}, | ||
| {path: "dbfs:/mnt/path/to/package", expected: false}, | ||
| @@ -86,3 +87,51 @@ func TestIsLibraryLocal(t *testing.T) { | ||
| require.Equalf(t, tc.expected, IsLibraryLocal(tc.path), "failed case: %d, path: %s", i, tc.path) | ||
| } | ||
| } | ||
| func TestIsLocalRequirementsFile(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| input string | ||
| expected string | ||
| isLocal bool | ||
| }{ | ||
| { | ||
| name: "valid requirements file with space", | ||
| input: "-r requirements.txt", | ||
andrewnester marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| expected: "requirements.txt", | ||
| isLocal: true, | ||
| }, | ||
| { | ||
| name: "remote requirements file", | ||
| input: "-r /Workspace/Users/requirements.txt", | ||
| expected: "/Workspace/Users/requirements.txt", | ||
| isLocal: false, | ||
| }, | ||
| { | ||
| name: "not a requirements file", | ||
| input: "some.txt", | ||
| expected: "", | ||
| isLocal: false, | ||
| }, | ||
| { | ||
| name: "-r with no space", | ||
| input: "-rrequirements.txt", | ||
| expected: "", | ||
| isLocal: false, | ||
| }, | ||
| { | ||
| name: "empty string", | ||
| input: "", | ||
| expected: "", | ||
| isLocal: false, | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| got, isLocal := IsLocalRequirementsFile(tt.input) | ||
| require.Equal(t, tt.expected, got) | ||
| require.Equal(t, tt.isLocal, isLocal) | ||
| }) | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.