Add JobTaskClusterSpec validate mutator #1784

kanterov · 2024-09-23T10:25:13Z

Changes

Add JobTaskClusterSpec validate mutator. It catches the case when tasks don't which cluster to use.

For example, we can get this error with minor modifications to default-python template:

      tasks:
        - task_key: python_file_task
          spark_python_task:
            python_file: ../src/my_project_10/main.py

 % databricks bundle validate
Error: Missing required cluster or environment settings
  at resources.jobs.my_project_10_job.tasks[0]
  in resources/my_project_10_job.yml:17:11

Task "print_github_stars" requires a cluster or an environment to run.
Specify one of the following fields: job_cluster_key, environment_key, existing_cluster_id, new_cluster.

We implicitly rely on "one of" validation, which does not exist. Many bundle fields can't co-exist, for instance, specifying: JobTask.{existing_cluster_id,job_cluster_key}, Library.{whl,pypi}, JobTask.{notebook_task,python_wheel_task}, etc.

Tests

Unit tests

bundle/config/validate/job_task_cluster_spec.go

pietern · 2024-09-23T11:26:25Z

bundle/config/validate/job_task_cluster_spec.go

+
+			diags = diags.Extend(validateJobTask(rb, task, taskPath))
+		}
+	}


This mixes access to the typed configuration with dyn.Value access.

Any chance we can just use the dyn.Value one?

I can change it. I think it's easier to implement and test validations using typed configuration though. The only problematic part is constructing path to a property.

What I was thinking is you can use dyn.MapByPattern to find all tasks across all jobs (and you get the right base path), and then use convert.ToTyped to get the typed jobs.Task value.

pietern

The code LGTM, but not sure about the classic/serverless ambiguity in the errors.

bundle/config/validate/job_task_cluster_spec.go

pietern · 2024-09-24T11:52:02Z

bundle/config/validate/job_task_cluster_spec_test.go

+				TaskKey:         "my_task",
+			},
+			errorPath:    "resources.jobs.job1.tasks[0]",
+			errorDetail:  "Task \"my_task\" has a task type that requires a cluster, but no cluster is specified",


What I find tricky (and counterintuitive) is that the error detail talks about a cluster, but the user's underlying intent may be to use serverless and an environment_key.

How can we surface this ambiguity in the error/warning? E.g. an and/or?

The error summary string contains one of possible attributes to set:

Error: One of the following fields must be set: job_cluster_key, environment_key, existing_cluster_id, new_cluster at resources.jobs.my_project_10_job.tasks[0] in resources/my_project_10_job.yml:17:11 Task "python_file_task" uses a task type that requires a cluster

I can change the wording to:

Task "my_task" has a task type that requires a cluster or environment, but neither is specified.

pietern · 2024-09-24T11:55:18Z

bundle/config/validate/job_task_cluster_spec.go

+
+			diags = diags.Extend(validateJobTask(rb, task, taskPath))
+		}
+	}


What I was thinking is you can use dyn.MapByPattern to find all tasks across all jobs (and you get the right base path), and then use convert.ToTyped to get the typed jobs.Task value.

kanterov · 2024-09-25T06:29:57Z

@pietern I've updated error/warning messages and added more comments as you suggested. I assume that the convert.ToTyped comment is optional because, at this point, the module is small and logic is encapsulated. I think it makes a lot of sense to have shared visitors, for instance, extract code that visits all tasks that need cluster/environment.

pietern · 2024-09-25T06:47:23Z

Yes, it's not strictly necessary.

A higher-level comment about the error messages is that the summary is what users will read first, and that's what should contain what is wrong, not the solution. Right now they are swapped. The solution is in the summary and the error is in the detail. Could you swap those around? Then it will say, Error: task "main_task" has a task type that requires a cluster or environment, but neither is specified with the solution following it. WDYT?

pietern

Thanks!

Added one more suggestion.

bundle/config/validate/job_task_cluster_spec.go

Co-authored-by: Pieter Noordhuis <[email protected]>

kanterov · 2024-09-25T11:29:35Z

@pietern Thanks. I've updated the doc. Can you please add it to the merge queue?

Bundles: * Added support for creating all-purpose clusters ([#1698](#1698)). * Reduce time until the prompt is shown for bundle run ([#1727](#1727)). * Use Unity Catalog for pipelines in the default-python template ([#1766](#1766)). * Add verbose flag to the "bundle deploy" command ([#1774](#1774)). * Fixed full variable override detection ([#1787](#1787)). * Add sub-extension to resource files in built-in templates ([#1777](#1777)). * Fix panic in `apply_presets.go` ([#1796](#1796)). Internal: * Assert tokens are redacted in origin URL when username is not specified ([#1785](#1785)). * Refactor jobs path translation ([#1782](#1782)). * Add JobTaskClusterSpec validate mutator ([#1784](#1784)). * Pin Go toolchain to 1.22.7 ([#1790](#1790)). * Modify SetLocation test utility to take full locations as argument ([#1788](#1788)). * Simplified isFullVariableOverrideDef implementation ([#1791](#1791)). * Sort tasks by `task_key` before generating the Terraform configuration ([#1776](#1776)). * Trim trailing whitespace ([#1794](#1794)). * Move trampoline code into trampoline package ([#1793](#1793)). * Rename `RootPath` -> `BundleRootPath` ([#1792](#1792)). API Changes: * Changed `databricks apps delete` command to return . * Changed `databricks apps deploy` command with new required argument order. * Changed `databricks apps start` command to return . * Changed `databricks apps stop` command to return . * Added `databricks temporary-table-credentials` command group. * Added `databricks serving-endpoints put-ai-gateway` command. * Added `databricks disable-legacy-access` command group. * Added `databricks account disable-legacy-features` command group. OpenAPI commit 6f6b1371e640f2dfeba72d365ac566368656f6b6 (2024-09-19) Dependency updates: * Upgrade to Go SDK 0.47.0 ([#1799](#1799)). * Upgrade to TF provider 1.52 ([#1781](#1781)). * Bump golang.org/x/mod from 0.20.0 to 0.21.0 ([#1758](#1758)). * Bump github.com/hashicorp/hc-install from 0.7.0 to 0.9.0 ([#1772](#1772)).

Add JobTaskClusterSpec validate mutator

d7da8f1

pietern reviewed Sep 23, 2024

View reviewed changes

kanterov requested a review from pietern September 24, 2024 08:16

pietern reviewed Sep 24, 2024

View reviewed changes

Improve messages

d3032c4

kanterov requested a review from pietern September 24, 2024 15:40

kanterov added 2 commits September 25, 2024 10:26

Better messages

3a3569e

Fix lint

3a28d20

pietern approved these changes Sep 25, 2024

View reviewed changes

bundle/config/validate/job_task_cluster_spec.go Outdated Show resolved Hide resolved

kanterov and others added 2 commits September 25, 2024 13:19

Update bundle/config/validate/job_task_cluster_spec.go

3b49d5e

Co-authored-by: Pieter Noordhuis <[email protected]>

Fix tests

434aa17

pietern added this pull request to the merge queue Sep 25, 2024

Merged via the queue into databricks:main with commit 3d9decd Sep 25, 2024
5 checks passed

andrewnester mentioned this pull request Oct 1, 2024

[Release] Release v0.229.0 #1801

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JobTaskClusterSpec validate mutator #1784

Add JobTaskClusterSpec validate mutator #1784

kanterov commented Sep 23, 2024 •

edited by pietern

Loading

pietern Sep 23, 2024

kanterov Sep 23, 2024

pietern Sep 24, 2024

pietern left a comment

pietern Sep 24, 2024

kanterov Sep 24, 2024

pietern Sep 24, 2024

kanterov commented Sep 25, 2024

pietern commented Sep 25, 2024

pietern left a comment

kanterov commented Sep 25, 2024

Add JobTaskClusterSpec validate mutator #1784

Add JobTaskClusterSpec validate mutator #1784

Conversation

kanterov commented Sep 23, 2024 • edited by pietern Loading

Changes

Tests

pietern Sep 23, 2024

Choose a reason for hiding this comment

kanterov Sep 23, 2024

Choose a reason for hiding this comment

pietern Sep 24, 2024

Choose a reason for hiding this comment

pietern left a comment

Choose a reason for hiding this comment

pietern Sep 24, 2024

Choose a reason for hiding this comment

kanterov Sep 24, 2024

Choose a reason for hiding this comment

pietern Sep 24, 2024

Choose a reason for hiding this comment

kanterov commented Sep 25, 2024

pietern commented Sep 25, 2024

pietern left a comment

Choose a reason for hiding this comment

kanterov commented Sep 25, 2024

kanterov commented Sep 23, 2024 •

edited by pietern

Loading