Repository navigation
Conversation
cb28c17 to
b955c4d
Compare
Co-authored-by: Isaac <no-reply@databricks.com>
Integration test reportCommit: 4df1dd5
Top 3 slowest tests (at least 2 minutes):
|
…atabricks#7085) ## Changes Twenty-one new `bundle validate` acceptance tests, each pinning current behaviour for a configuration edge case. They are grouped by topic under `acceptance/bundle/validate/` (`yaml_anchors/` for anchors and merge keys, `yaml_duplicate_keys/`, `variables/`, `targets/`, `includes/`, `nulls/`, `references/`); ten existing tests on the same topics moved into the groups. - merge keys and aliases in `targets.*.variables` (inherited through a target or root `<<`, aliases of a complex default, duplicate and malformed `<<`, inline mappings), - duplicate keys (with the processed JSON), timestamps as shorthands or nested in complex defaults, - references that resolve to the wrong type inside lists and maps, - includes overriding fields with `""` / `0` / `false`, diagnostics after a failed include merge, - `resources: null`, `targets: false`, `schedule: null`, - diagnostic locations for unknown fields in complex variables and duplicated app resources. `Badness` marks the cases where current behaviour is itself wrong: - a timestamp nested in a complex default fails with an internal error; a timestamp shorthand is dropped; - an inline mapping as the merge value is rejected although the YAML spec allows it; - duplicate keys are accepted silently (the later one wins); - an empty `resources:` section is an error without a location (empty `targets`, `variables`, `sync` are fine); `targets: false` fails with an internal "failed to rewrite" message; - an explicitly null `schedule` is reported as missing its required fields; - a failed include merge reports an error without a location, and hides the diagnostics of includes loaded after it. ## Why They guard behaviour ahead of databricks#7098, which replaces `libs/dyn`: where it differs, the test shows it. ## Tests Goldens recorded on main. This pull request and its description were written by Isaac. --------- Co-authored-by: Isaac <no-reply@databricks.com>
|
Commit: Regression test reportSelected 20 changed tests (limit=20, 7 not selected) 40 tests: regression=34 unreleased=0 coverage=6 failing=0
TestAccept/bundle/override/preset_clears_forced_schedule_reference/DMS=: branch ✅ | main (bef6321) ❌ | latest (v1.21.0) ❌main (bef6321): latest (v1.21.0): same output as main (bef6321) TestAccept/bundle/override/preset_clears_forced_schedule_reference/DMS=true: branch ✅ | main (bef6321) ❌ | latest (v1.21.0) ❌main (bef6321): latest (v1.21.0): TestAccept/bundle/validate/app_resource_duplicate_name_location/DMS=: branch ✅ | main (bef6321) ❌ | latest (v1.21.0) ❌main (bef6321): latest (v1.21.0): same output as main (bef6321) TestAccept/bundle/validate/app_resource_duplicate_name_location/DMS=true: branch ✅ | main (bef6321) ❌ | latest (v1.21.0) ❌main (bef6321): latest (v1.21.0): same output as main (bef6321) TestAccept/bundle/validate/includes/merge_failure_short_circuits_diagnostics/DMS=: branch ✅ | main (bef6321) ❌ | latest (v1.21.0) ❌main (bef6321): latest (v1.21.0): same output as main (bef6321) TestAccept/bundle/validate/includes/merge_failure_short_circuits_diagnostics/DMS=true: branch ✅ | main (bef6321) ❌ | latest (v1.21.0) ❌main (bef6321): latest (v1.21.0): same output as main (bef6321) TestAccept/bundle/validate/nulls/resources_container/DMS=: branch ✅ | main (bef6321) ❌ | latest (v1.21.0) ❌main (bef6321): latest (v1.21.0): same output as main (bef6321) TestAccept/bundle/validate/nulls/resources_container/DMS=true: branch ✅ | main (bef6321) ❌ | latest (v1.21.0) ❌main (bef6321): latest (v1.21.0): same output as main (bef6321) TestAccept/bundle/validate/references/resolved_invalid_sequence_items/DMS=: branch ✅ | main (bef6321) ❌ | latest (v1.21.0) ❌main (bef6321): latest (v1.21.0): same output as main (bef6321) TestAccept/bundle/validate/references/resolved_invalid_sequence_items/DMS=true: branch ✅ | main (bef6321) ❌ | latest (v1.21.0) ❌main (bef6321): latest (v1.21.0): same output as main (bef6321) TestAccept/bundle/validate/references/to_omitted_struct_defaults/DMS=: branch ✅ | main (bef6321) ❌ | latest (v1.21.0) ❌main (bef6321): latest (v1.21.0): same output as main (bef6321) TestAccept/bundle/validate/references/to_omitted_struct_defaults/DMS=true: branch ✅ | main (bef6321) ❌ | latest (v1.21.0) ❌main (bef6321): latest (v1.21.0): same output as main (bef6321) TestAccept/bundle/validate/required/DMS=: branch ✅ | main (bef6321) ❌ | latest (v1.21.0) ❌main (bef6321): latest (v1.21.0): same output as main (bef6321) TestAccept/bundle/validate/required/DMS=true: branch ✅ | main (bef6321) ❌ | latest (v1.21.0) ❌main (bef6321): latest (v1.21.0): same output as main (bef6321) TestAccept/bundle/validate/targets/inherited_from_root_merge/DMS=: branch ✅ | main (bef6321) ❌ | latest (v1.21.0) ❌main (bef6321): latest (v1.21.0): same output as main (bef6321) TestAccept/bundle/validate/targets/inherited_from_root_merge/DMS=true: branch ✅ | main (bef6321) ❌ | latest (v1.21.0) ❌main (bef6321): latest (v1.21.0): same output as main (bef6321) TestAccept/bundle/validate/targets/scalar_rewrite_diagnostic/DMS=: branch ✅ | main (bef6321) ❌ | latest (v1.21.0) ❌main (bef6321): latest (v1.21.0): same output as main (bef6321) TestAccept/bundle/validate/targets/scalar_rewrite_diagnostic/DMS=true: branch ✅ | main (bef6321) ❌ | latest (v1.21.0) ❌main (bef6321): latest (v1.21.0): TestAccept/bundle/validate/variables/complex_default_nested_timestamp/DMS=: branch ✅ | main (bef6321) ❌ | latest (v1.21.0) ❌main (bef6321): latest (v1.21.0): same output as main (bef6321) TestAccept/bundle/validate/variables/complex_default_nested_timestamp/DMS=true: branch ✅ | main (bef6321) ❌ | latest (v1.21.0) ❌main (bef6321): latest (v1.21.0): same output as main (bef6321) 14 more failure details omitted, see the full report in the run. |
857e2d4 to
b9d8067
Compare
Co-authored-by: Isaac <no-reply@databricks.com>
## Changes
Add `ForceSendFields` to `Presets`, `Lifecycle` and `Deployment`, so
explicit zero values (`false`, `0`, `""`) written in a target or include
replace non-zero base values.
Fixes on main: `targets.dev.resources.jobs.j.lifecycle.prevent_destroy:
false` was ignored and the base `true` stayed.
Visible change: `bundle validate -o json` now shows explicit zero
presets (e.g. `name_prefix: ""`) instead of `presets: {}`.
Not added to `Target`, `Workspace`, `Bundle`, `Experimental`, `Lock`,
`Artifact` or the other structs: they have pointer/map/slice fields, and
`ForceSendFields` there turns explicit `null` overrides (e.g. `run_as:`)
into forced values.
Prerequisite for removing `libs/dyn` (databricks#7098).
## Tests
- New `validate/lifecycle_override`: fails on main, passes here.
- `validate/includes/clears_unforced_preset_values` golden updated.
- Both pass on the databricks#7098 branch with this patch applied.
This pull request and its description were written by Isaac.
---------
Co-authored-by: Isaac <no-reply@databricks.com>
Replace the generated pattern tables, matched against every node of the dynamic configuration with a trie, by generated walkers that descend only along the patterns' paths of the typed configuration. Null, zero, pure-reference values and locations still come from the dyn.Value at each node (walk_node.go), so the diagnostics are unchanged. Slice of #7098. Co-authored-by: Isaac <no-reply@databricks.com>
Replace the generated pattern tables, matched against every node of the dynamic configuration with a trie, by generated walkers that descend only along the patterns' paths of the typed configuration. Null, zero, pure-reference values and locations still come from the dyn.Value at each node (walk_node.go), so the diagnostics are unchanged. Slice of #7098. Co-authored-by: Isaac <no-reply@databricks.com>
d95e7b1 to
19dde68
Compare
Co-authored-by: Isaac <no-reply@databricks.com>
Replace the generated pattern tables, matched against every node of the dynamic configuration with a trie, by generated walkers that descend only along the patterns' paths of the typed configuration. Null, zero, pure-reference values and locations still come from the dyn.Value at each node (walk_node.go), so the diagnostics are unchanged. Slice of #7098. Co-authored-by: Isaac <no-reply@databricks.com>
19dde68 to
e82320b
Compare
Co-authored-by: Isaac <no-reply@databricks.com>
NewPattern builds a pattern from parts like NewPath does for paths, so callers don't nest the single-step constructors or parse pattern strings. Co-authored-by: Isaac <no-reply@databricks.com>
…r refs and locations
Bundle configuration is the typed struct plus structvar (pure ${...}
references in non-string fields and a tree of source locations). YAML and
JSON decode straight into the structs; mutators, validators, variable
resolution, configsync and generate work on structvar views and structpath
paths. libs/dyn is deleted.
Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Top level fields and resource groups come from the locations tree, resources and job tasks from the typed configuration, instead of a pattern traversal of views. SelectTarget deletes targets and environments (with their locations) rather than only clearing them. Co-authored-by: Isaac <no-reply@databricks.com>
ResourceKeySet.AddResources/AddResourcesOfType iterate the typed resources instead of matching a pattern over a view; the backwards-compatibility copy of 'python' to 'experimental.python' is a field assignment plus the locations subtree. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
…ngs walker The scope (whole configuration with or without resources, resources only, variable lookups) selects a walk.Strings entry point instead of a pattern matched over a view; the reflective string walk in hasReferencesToResolve goes away. Resolution itself still looks up arbitrary references in the view. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Replace View lookups and reflective walks where the types are known: - validate_engine, secret value, auth interpolation: plain field access with the presence rules (ForceSendFields, written, references); - environments -> targets: field assignment plus Root.MoveMetadata; - apps: Resources.PermissionsOf; plan prevent_destroy: Resources.GetResource (JobRun gets the GetLifecycle override it was missing); - DropEmptyStrings, CollectEscapeTelemetry: walk.Strings modes EmptyFields and NonEmpty; AllResourcesHaveValues: IterateAllResources; - selectResources: Resources.Select/CopyResource plus Root.CopyMetadata; - direct extractReferences and ResolveConfigAgainstState: walk.ResourceStringsOf plus Root.ReferencesUnder; - SetVariables iterates the typed variables and copies the default from a view of the field. The resources iterator generator is now a text/template. Co-authored-by: Isaac <no-reply@databricks.com>
- the python -> experimental.python copy carries references and copies the slices (Root.CopyMetadataAt; MoveMetadata uses it); - MergeGrants uses IterateAllGrants; - keyed merges and reference checks take built paths (IsReferenceAt); Root.IsReference(string) is gone; - ReferencesUnder(nil) covers every reference; - SingleNodeCluster no longer stores a context; - resolveIn is a regular function; - defaults_walk.go is registered as generated; - remove structvar.ForEach and structpath.MustParsePattern, now unused. Co-authored-by: Isaac <no-reply@databricks.com>
Locations are for diagnostics and path resolution. View, the generated
required/enum/defaults walkers and the mutators no longer treat a value
with a location as present; a value is present if it is not zero, is
forced, or holds a pure reference. Decoding records written zero values
in ForceSendFields, so they stay present where the struct has one.
Decisions:
- Required: an empty string never satisfies a required field, even if
forced; a forced false/0 does (zero is meaningful for bools and numbers).
So name: "" warns, and job_id: 0 in run_job_task warns (RunJobTask has
no ForceSendFields; a job id 0 is invalid anyway).
- Defaults keep a forced "" (the user asked for an empty value).
- Lost: a written zero in a struct without ForceSendFields and an empty
struct value (resources: {} -> summary .resources is null); an explicit
source: "" on spark_python_task/sql_task.file (no ForceSendFields) gets
the GIT default; development: true under mode: development no longer
warns (indistinguishable from the preset).
- workspace.file_path with immutable folders is rejected in
DefineDefaultWorkspacePaths, before the default is set, instead of by
checking its location.
- Dashboard required checks report by paths, not by locations found.
Co-authored-by: Isaac <no-reply@databricks.com>
Revised decision for required fields: a string is set if it is non-empty or forced, like every other scalar. An empty string is a valid value (the built-in SQL template has a job parameter whose default resolves to ""). A written zero in a struct without ForceSendFields cannot be told apart from unset, so it is reported missing (job_id: 0 in run_job_task warns); JobParameterDefinition.default is exempt because an empty default is valid. Also: - a struct whose only set fields are references is present (Node.HasRefs), in required checks and in the required/defaults traversal; - workspace.file_path with immutable folders is rejected in TranslatePaths again, after variable resolution, by value: any value other than the two defaults DefineDefaultWorkspacePaths sets is the user's; - remote_path and StateToBundle no longer use locations for presence; Resources.IsZero is gone; - resolve_config builds the state key from the path; - SingleNodeCluster checks clusters as it visits them, ctx as an argument. Co-authored-by: Isaac <no-reply@databricks.com>
… slices Most mappings are small: search their keys linearly and index only from 16 keys. Keep the key location as a value and build the slice only for a diagnostic. Load allocates 11% fewer bytes and 13% fewer objects. Co-authored-by: Isaac <no-reply@databricks.com>
- Root.References/ReferencesUnder skip references whose path is no longer
in the typed value, e.g. below a new_cluster that OverrideCompute cleared
for bundle.cluster_id; deploying used to fail resolving them ("parent is
nil pointer"). References is ReferencesUnder(nil).
- An explicit libraries: null on a pipeline (forced) errors again, as on main.
- Reference keys are rebased with path operations, not string concatenation.
Co-authored-by: Isaac <no-reply@databricks.com>
structaccess.Get treats a terminal nil pointer as reachable and skips
readonly/internal fields, so a new_cluster: ${var.cluster} that
OverrideCompute cleared came back, and bundle.terraform: ${var.x} was
dropped. A reference is now kept if the view finds its path: a nil
pointer (decoding allocates one for a reference) hides it.
Co-authored-by: Isaac <no-reply@databricks.com>
- A reference is kept only if the view shows it (a string): a forced nil pointer, e.g. a schedule: null that a target override referenced and a mutator then cleared, hides it too. The acceptance test covers the scenario (the test server does not reproduce the stale reference). - Set no longer adds an empty location node to mark a value present: presence does not use locations, and it cloned every ancestor on each default write. Co-authored-by: Isaac <no-reply@databricks.com>
- The generated strings walker does not descend into a field that holds a
pure reference, as the view does: with tags: ${...} and preset tags added
to the typed map, the hidden entries no longer become dependencies (a job
referred to itself and planning failed with a cycle).
- The immutable-folder file_path check cleans the value: a root_path ending
in / resolves the default to root//files.
- Scalars in dynamic values are not copied reflectively by the strings walk.
Co-authored-by: Isaac <no-reply@databricks.com>
- Every generated struct walker returns when its node holds a pure reference, so a referenced map entry, slice element or resource (also in the per-resource dispatch) hides what is below it, as in the view. - Scoped walks (resources, one resource, variable lookups) build the reference tree from the references in their scope only. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Keeps the diff against main small: the string forms stay, as thin wrappers over GetLocationOf/GetLocationsOf (the nearest-location lookups), and the call sites that used them go back to main's text. The exact-location call sites stay on GetLocationOfExact. Moving the remaining callers to built paths is left for a follow-up. Co-authored-by: Isaac <no-reply@databricks.com>
visitArgs.path is a built PathNode and the skip predicate is checked by the dependency loops before building it. Candidates that are mostly empty (the whl, jar and requirements of every library) check presence before building the path. Same speed on TranslatePaths and NormalizePaths, 15% fewer allocations, and no closure at the call sites. Co-authored-by: Isaac <no-reply@databricks.com>
#7085 recorded main's behaviour. Where the typed configuration fixes a wrong behaviour the Badness is gone (nested and shorthand timestamps, an inline merge mapping, resources: null, a scalar targets section). Where it behaves differently in a wrong way a Badness records it: scalar target variable overrides inherited through a merge key, an aliased complex default rewritten in place, duplicate and invalid merge keys, a duplicate default key, wrong types turned into empty strings, the lost location of an unknown field in a variable default, and presets that an include cannot clear. Co-authored-by: Isaac <no-reply@databricks.com>
#7117 added ForceSendFields to Presets, so a written zero is recorded. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
…le and Deployment Co-authored-by: Isaac <no-reply@databricks.com>
…ong type Assign zeroed the slot, which left an empty string in lists and maps like main did not. Delete now removes slice elements, and resolution applies the updates from the last element to the first. Co-authored-by: Isaac <no-reply@databricks.com>
…e schedule warnings as Badness
…nerated code for the typed configuration Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Replace the generated pattern tables, matched against every node of the dynamic configuration with a trie, by generated walkers that descend only along the patterns' paths of the typed configuration. Null, zero, pure-reference values and locations still come from the dyn.Value at each node (walk_node.go), so the diagnostics are unchanged. Slice of #7098. Co-authored-by: Isaac <no-reply@databricks.com>
e82320b to
4df1dd5
Compare
Changes
Bundle configuration (
databricks.ymland its includes) was held as a dynamic value tree (libs/dyn), and every mutator converted the typedconfig.Rootto that tree and back. On large bundles the conversion dominated command time (about 90 ms around every mutator on a 1000-job bundle). This PR deleteslibs/dyn(and the Terraform convertertfdyn) and keeps the configuration only as typed Go structs.config.Rootis the typed struct plus two side structures: pure${...}references in fields that cannot hold a string (e.g.max_retries: ${var.n}) and a tree of source locations.libs/structs/structvarprovides what the tree used to: decoding YAML/JSON straight into the structs (same warnings), a read-onlyView, writes, merge/override, and variable resolution with the same semantics and error text asdynvar. Paths arestructpatheverywhere, includingdiag.Diagnostic.Paths.generatecommands write YAML with the newlibs/structs/structyaml(identical output).go run ./bundle/internal/validation: the required and enum walkers (only the paths that have rules; introduced in validate: check required and enum fields with generated typed walkers #7143 with a small bridge todyn, which this PR replaces with the typed references and locations), resource defaults, a strings walker (strings with${, non-empty strings, forced-empty fields), and resource iterators inconfig(IterateAllResources/Permissions/Grants/ResourceStates,GetResource,PermissionsOf,Select,CopyResource). No pattern-based traversal of the typed configuration is left (structvar.ForEachis gone);Viewis only used where the data is dynamic: decode, merge/override, Python input and output, configsync, resolving arbitrary${...}paths, JSON output.ForceSendFields, never from locations. A value is present if it is non-zero, listed in its struct'sForceSendFields, or a pure reference; decoding records a written zero value inForceSendFields. What this loses:ForceSendFieldsis indistinguishable from unset, e.g.job_id: 0inrun_job_tasknow warns as not set;presets: {},resources: {}) is absent;source: ""onspark_python_task/sql_task.filegets theGITdefault on git_source jobs;developmentdeprecation warning is not shown fordevelopment: trueundermode: development(indistinguishable from the preset).Root.GetLocationOf/GetLocationsOfkeep main's names and return the closest location;...Exactvariants return where a value is written.GetLocation(path string)stays as on main.Why
Measured on a 16-core Mac, 1000-job bundle (
./task bench-1k, test server, wall time and peak RSS), main (4244322a26) vs #6954 vs this PR:bundle validatebundle planbundle deploy(User CPU for
validate: 20.9 s → 1.56 s → 0.70 s.deploywith DMS at 1000 jobs fails on main: the test server caps a deployment version at 800 operations.)Per component, ms for one
bundle.Applyon the same bundle (bundle/internal/bundletest/component_benchmark_test.go, which only uses APIs that also exist on main; main additionally pays ~89 ms of conversion around every mutator):The full acceptance suite takes the same time on all three, so the gain shows in commands on large bundles, not in the test suite (16-core Mac, interleaved runs: main 206-237 s, #6954 199-217 s, this PR 216-221 s).
Tests
Unit and acceptance tests. #7085 (merged) recorded main's behaviour in 21
bundle validateedge cases (acceptance/bundle/validate/{yaml_anchors,yaml_duplicate_keys,variables,targets,includes,nulls,references}); this PR updates the expectations of 19 of them, andBadnessmarks wrong behaviour:<<mapping is accepted;resources: nullis accepted andtargets: falseis a warning with a location instead of an internal error; diagnostics of includes after a failed merge are reported; an include can clear a preset with""/0/false(with config: ForceSendFields on CLI config structs #7117); a reference that resolves to the wrong type drops the list element or map entry, as on main, instead of leaving"";<<merge are not applied; an alias of a complex default is rewritten in place; a duplicate<<key and a scalar among merged maps are accepted; a duplicatedefaultkey in a target variable becomes the default value; the location of an unknown field in a complex variable default is lost; required-field warnings for a whole struct that is a reference to a complete struct (quality_monitorsschedule); an explicit0in a required field withoutForceSendFields(e.g.health.rules[].value: 0) is reported as not set.Difference from #6954
#6954 is the first version of this change and shares its commits: same typed model, same deleted packages. Its mutators still traverse the typed configuration through reflection (
structvar.Viewlookups and pattern-basedForEach/Walk), so it is 7-10x faster than main on large bundles but not faster for every component: Enum and Required barely improve and Load is slower than main (table above). This PR replaces those traversals with generated walkers and plain field access, which makes every component faster than main except Load, which is at parity, andvalidate/plananother 2.2x anddeploy1.45x faster than #6954. It decides presence byForceSendFieldsonly, where #6954 also used locations. Relative to #7143 this PR changes about 9.2k/9.9k lines (added/removed) of hand-written non-test Go, plus 26k lines of generated walkers.This pull request and its description were written by Isaac.