Added support for artifacts building for bundles #583

andrewnester · 2023-07-18T10:41:53Z

Changes

Added support for artifacts building for bundles.

Now it allows to specify artifacts block in bundle.yml and define a resource (at the moment Python wheel) to be build and uploaded during bundle deploy

Built artifact will be automatically attached to corresponding job task or pipeline where it's used as a library

Follow-ups:

If artifact is used in job or pipeline, but not found in the config, try to infer and build it anyway
If build command is not provided for Python wheel artifact, infer it

Tests

Case 1. Explicitly defined libraries block

bundle.yml

bundle:
  name: wheel-task

artifacts:
    my_test_code:
      type: whl
      build: "/usr/local/bin/python setup.py bdist_wheel"

workspace:
  host: https://******.com

resources:
  jobs:
    test_job:
      name: "[${bundle.environment}] My Wheel Job"
      tasks:
        - task_key: TestTask
          existing_cluster_id: "0717-*****-********"
          python_wheel_task:
            package_name: "my_test_code"
            entry_point: "run"
          libraries:
          - whl: ./dist/my_test_code-*.whl

andrew.nester@HFW9Y94129 wheel % cli bundle deploy
artifacts.whl.Build(my_test_code): Building...
artifacts.whl.Build(my_test_code): Build succeeded
Starting upload of bundle files
Uploaded bundle files at /Users/andrew.nester@databricks.com/.bundle/wheel-task/default/files!

artifacts.Upload(my_test_code-0.0.1-py2-none-any.whl): Uploading...
artifacts.Upload(my_test_code-0.0.1-py2-none-any.whl): Upload succeeded
Starting resource deployment
Resource deployment completed!

Python wheel was successfully built and uploaded and attached to matching job.

Case 2. Explicitly defined libraries block but with non matching path

....
          python_wheel_task:
            package_name: "my_test_code"
            entry_point: "run"
          libraries:
          - whl: ./dist/not-exists-*.whl
...

artifacts.whl.Build(my_test_code): Building...
artifacts.whl.Build(my_test_code): Build succeeded
Starting upload of bundle files
Uploaded bundle files at /Users/andrew.nester@databricks.com/.bundle/wheel-task/default/files!

Error: no file matches found for library ./dist/notexists-*.whl

Case 3. No libraries block, artifact is not used

....
          python_wheel_task:
            package_name: "my_test_code"
            entry_point: "run"
...

artifacts.whl.Build(my_test_code): Building...
artifacts.whl.Build(my_test_code): Build succeeded
Starting upload of bundle files
Uploaded bundle files at /Users/andrew.nester@databricks.com/.bundle/wheel-task/default/files!

Error: task 'TestTask' is missing required libraries. Please include your package code in task libraries block

Integration test

2023/07/20 17:28:19 [INFO] Locating tests in /Users/andrew.nester/cli
2023/07/20 17:28:19 [INFO] found test in /Users/andrew.nester/cli/bundle/tests/bundle
...
=== RUN   TestAccBundlePythonWheelBuild
    wheel_test.go:15
2023/07/20 17:28:25 INFO Phase: build
artifacts.whl.Build(my_test_code): Building...
artifacts.whl.Build(my_test_code): Build succeeded
--- PASS: TestAccBundlePythonWheelBuild (0.29s)
PASS
ok      github.com/databricks/cli/bundle/tests/bundle   1.063s

lennartkats-db · 2023-07-18T11:11:58Z

Nice! For your example, there doesn't seem to be a whl library referenced in the job?

andrewnester · 2023-07-18T11:19:13Z

@lennartkats-db you're right, we do not need to explicitly reference the library in the same manner it is now in DBX. What happens underneath is that CLI check which package is being used in a job, finds corresponding artefact and attaches it as a library

https://github.com/databricks/cli/pull/583/files#diff-80cac01f5e27a35089510718bd80e3d1964b4c2c4eac9d9eb7b283a0f6a83c3cR38

lennartkats-db · 2023-07-18T11:33:12Z

That seems like it's a bit too magical to me... It would become really hard to find out what is used in which jobs if you have a DAB with several (>1) jobs. I'd rather have an explicit reference to the library just like we always have in job definitions for libraries.

To me it would be ideal if the job had an explicit reference to a whl file, just like it does today, without even using variables. That way we could even support visual editing of such a job.

bundle/artifacts/whl/build.go

bundle/libraries/libraries.go

python/utils.go

pietern · 2023-07-20T09:47:10Z

@lennartkats-db It's not unexpected though. If I specify a package foo in my job, and I have a wheel definition for a package foo in my local directory, that this commands puts them together and makes it work. If you have >1 jobs or >1 wheels the same still works and you can unambiguously refer to either wheel.

pietern

Please add an integration test with a minimal setup.py just to check that performing the build works if the parameters are set correctly. I suspect it would have caught an issue with python.FindFileWithSuffixInPath("dist", ".whl").

bundle/artifacts/whl/build.go

bundle/config/artifact.go

bundle/libraries/libraries.go

pietern

Last couple of comments that I didn't catch in the previous round.

Everything else LGTM.

bundle/artifacts/artifacts.go

bundle/artifacts/build.go

bundle/config/artifact.go

bundle/libraries/libraries.go

lennartkats-db

I'm really not comfortable with this amount of magic ... we should discuss what we want to do here. So far we have designed DABs to be explicit and have directly followed the API specs for the format. Automagically inserting a library dependency into one of the jobs seems contrary to that design and is contrary to the Python mantra of Explicit is better than Implicit. cc @pietern

bundle/artifacts/artifacts.go

bundle/artifacts/build.go

pietern · 2023-07-25T06:16:54Z

bundle/artifacts/upload.go

+	artifactPath := b.Config.Workspace.ArtifactsPath
+	b.WorkspaceClient().Workspace.Delete(ctx, workspace.Delete{
+		Path:      artifactPath,
+		Recursive: true,


Not too keen on a recursive delete without validation. If someone configures artifact_path to ~ then this would remove everything underneath. Could we do some kind of validation prior to a recursive delete, or better yet, do some kind of tracking which files we have uploaded and only remove those?

I'm not a big fan of tracking the uploaded files as it can be a complex and error prone in the end.
Instead I suggest to upload artifacts to subfolder of artifact_path and then we can safely delete it each time

That works. Agree file tracking would add a lot of complexity.

pietern · 2023-07-25T06:17:38Z

bundle/artifacts/whl/build.go

 		return fmt.Errorf("artifacts.whl.Build(%s): cannot find built wheel in %s", m.name, dir)
 	}
-	artifact.File = filepath.Join(dir, wheel)
+	for _, wheel := range wheels {


When can we expect multiple wheels?

For Python project I can imagine we might have some setup for monorepo and custom builds which yields multiple Python wheels.

In that case I'd expect multiple artifacts be defined, one for each wheel. But it's fine for symmetry with the Java case you describe below.

bundle/config/artifact.go

bundle/libraries/libraries.go

pietern

Case 3 is unexpected. Could we figure out a way to display a warning?

Addressed in the followup commits

Breaking Change: * Require include glob patterns to be explicitly defined ([#602](#602)). Bundles: * Add support for more SDK config options ([#587](#587)). * Add template renderer for Databricks templates ([#589](#589)). * Fix formatting in renderer.go ([#593](#593)). * Fixed python wheel test ([#608](#608)). * Auto detect Python wheel packages and infer build command ([#603](#603)). * Added support for artifacts building for bundles ([#583](#583)). * Add support for cloning repositories ([#544](#544)). * Add regexp compile helper function for templates ([#601](#601)). * Add unit test that raw strings are printed as is ([#599](#599)). Internal: * Fix tests under ./cmd/configure if DATABRICKS_TOKEN is set ([#605](#605)). * Remove dependency on global state in generated commands ([#595](#595)). * Remove dependency on global state for the root command ([#606](#606)). * Add merge_group trigger for build ([#612](#612)). * Added support for build command chaining and error on missing wheel ([#607](#607)). * Add TestAcc prefix to filer test and fix any failing tests ([#611](#611)). * Add url parse helper function for templates ([#600](#600)). * Remove dependency on global state for remaining commands ([#613](#613)). * Update CHANGELOG template ([#588](#588)).

Added support for artifacts building for bundles

df49259

andrewnester requested review from pietern and shreyas-goenka July 18, 2023 10:41

andrewnester mentioned this pull request Jul 18, 2023

Added build and upload steps for Python wheel artifacts #400

Closed

pietern reviewed Jul 20, 2023

View reviewed changes

dont use chdirAndBack

4d62ed7

andrewnester requested a review from pietern July 20, 2023 11:49

pietern reviewed Jul 20, 2023

View reviewed changes

bundle/artifacts/whl/build.go Outdated Show resolved Hide resolved

bundle/config/artifact.go Outdated Show resolved Hide resolved

bundle/libraries/libraries.go Outdated Show resolved Hide resolved

bundle/libraries/libraries.go Outdated Show resolved Hide resolved

pietern reviewed Jul 20, 2023

View reviewed changes

bundle/artifacts/artifacts.go Outdated Show resolved Hide resolved

bundle/artifacts/build.go Outdated Show resolved Hide resolved

bundle/config/artifact.go Outdated Show resolved Hide resolved

bundle/libraries/libraries.go Outdated Show resolved Hide resolved

lennartkats-db previously requested changes Jul 20, 2023

View reviewed changes

andrewnester added 3 commits July 20, 2023 17:11

added test for build

ab1d769

marked test as integration

54897c5

addressed comments

cea53c1

andrewnester requested a review from pietern July 20, 2023 15:29

explicit libraries block

bcf236d

andrewnester requested a review from lennartkats-db July 24, 2023 10:41

andrewnester added 4 commits July 24, 2023 12:49

fixed typo

60c7ca2

normalise workspace path

0532937

added hash for names and cleanup

5005469

artifact file can be used in multiple libraries

4420ee8

pietern reviewed Jul 25, 2023

View reviewed changes

addressed feedback

7f31c82

andrewnester requested a review from pietern July 25, 2023 08:44

pietern approved these changes Jul 25, 2023

View reviewed changes

andrewnester added 2 commits July 25, 2023 13:23

check for missing libraries

f5d5357

Merge branch 'main' into artifact-block

c9e9e8f

andrewnester merged commit 9a88fa6 into main Jul 25, 2023
4 checks passed

pietern deleted the artifact-block branch July 25, 2023 13:59

mgyucht mentioned this pull request Jul 27, 2023

Release v0.202.0 #619

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for artifacts building for bundles #583

Added support for artifacts building for bundles #583

andrewnester commented Jul 18, 2023 •

edited

Loading

lennartkats-db commented Jul 18, 2023

andrewnester commented Jul 18, 2023

lennartkats-db commented Jul 18, 2023

pietern commented Jul 20, 2023

pietern left a comment

pietern left a comment

lennartkats-db left a comment

pietern Jul 25, 2023

andrewnester Jul 25, 2023

pietern Jul 25, 2023

pietern Jul 25, 2023

andrewnester Jul 25, 2023

pietern Jul 25, 2023

pietern left a comment

Added support for artifacts building for bundles #583

Added support for artifacts building for bundles #583

Conversation

andrewnester commented Jul 18, 2023 • edited Loading

Changes

Tests

Case 1. Explicitly defined libraries block

Case 2. Explicitly defined libraries block but with non matching path

Case 3. No libraries block, artifact is not used

Integration test

lennartkats-db commented Jul 18, 2023

andrewnester commented Jul 18, 2023

lennartkats-db commented Jul 18, 2023

pietern commented Jul 20, 2023

pietern left a comment

Choose a reason for hiding this comment

pietern left a comment

Choose a reason for hiding this comment

lennartkats-db left a comment

Choose a reason for hiding this comment

pietern Jul 25, 2023

Choose a reason for hiding this comment

andrewnester Jul 25, 2023

Choose a reason for hiding this comment

pietern Jul 25, 2023

Choose a reason for hiding this comment

pietern Jul 25, 2023

Choose a reason for hiding this comment

andrewnester Jul 25, 2023

Choose a reason for hiding this comment

pietern Jul 25, 2023

Choose a reason for hiding this comment

pietern left a comment

Choose a reason for hiding this comment

andrewnester commented Jul 18, 2023 •

edited

Loading